Generate C++ coverage reports too! #2314

cinderblock · 2021-09-06T20:54:26Z

Coverage tests are wonderful.

This PR adds support for running coverage tests on this package's C++ sources alongside JavaScript's!

	Before	After	Change
Covered Lines	1 019	1 397	+ 37%
Uncovered Lines	96	1 219	+ 1 170%
Tracked Lines	1 115	2 616	+ 135%
Coverage	91%	53%	⚠️- 42%
Sunburst		Update: codecov broke this image!

Before

After

Update: codecov broke this image!

Platforms & Issues

Initially, I'd like to get coverage reports working on Linux and macOS. Windows looks like it will be a little more involved and could be added after initial merging of this PR.

macOS

Thoughts on the "correct" way to make coverage reports on macOS would be appreciated. While it works, it could probably be improved.

On Linux, apt install -y gcc also installs libgcov.a (via dependency libgcc-9-dev:amd64: /usr/lib/gcc/x86_64-linux-gnu/9/libgcov.a). I've tried installing gcovr and gcc with brew to see if they provide the missing libraries to no avail.

~~A CI test shows that libgcov.a is available on the system. So maybe it's just an LD_LIBRARY_PATH setting that needs to be set in bindings.gyp?~~

> find /usr -name 'libgcov.*'
/usr/local/lib/gcc/9/gcc/x86_64-apple-darwin19/9.4.0/libgcov.a
/usr/local/Cellar/gcc@10/10.3.0/lib/gcc/10/gcc/x86_64-apple-darwin19/10.3.0/libgcov.a
/usr/local/Cellar/gcc/11.2.0/lib/gcc/11/gcc/x86_64-apple-darwin19/11.2.0/libgcov.a
/usr/local/Cellar/gcc@9/9.4.0/lib/gcc/9/gcc/x86_64-apple-darwin19/9.4.0/libgcov.a

Note: the above was run after brew install gcc, which probably isn't necessary and probably added a few of these.

Update 1

Adding -L /usr/local/lib/gcc/9/gcc/x86_64-apple-darwin19/9.4.0 to the linker seems to have gotten it to compile on macOS, but when I run it, no gcda files seem to be generated... https://github.com/cinderblock/node-serialport/runs/3528420934?check_suite_focus=true

Update 2

Found a minimally working setup. Some improvements could probably be made.

                      'xcode_settings': {
                        'GCC_GENERATE_TEST_COVERAGE_FILES': ['YES'],
                        'GCC_INSTRUMENT_PROGRAM_FLOW_ARCS': ['YES'],
                        'OTHER_CFLAGS+': [
                          '-fprofile-arcs -ftest-coverage',
                        ],
                        'OTHER_LDFLAGS+': [
                          '-fprofile-arcs -ftest-coverage',
                          # There has to be a better way to do this...
                          '-L/usr/local/lib/gcc/9/gcc/x86_64-apple-darwin19/9.4.0',
                        ],
                      },

Windows

As for coverage tests on Windows, I'm not sure where to start. I'm sure there is an equivalent to gcc's gcov but I haven't had the time to look yet.

Update 1

Some possibly helpful tools/links:

✔️OpenCppCoverage
Official Instructions for Visual Studio - Probably not. Only works on Enterprise versions of Visual Studio.
Lcov for Windows
RKTracer - Paid
CPPCoverage - Maybe?

Update 2

CPPCoverage is running, but not generating coverage information yet for some reason.

npm run rebuild -- -- --debug
# https://github.com/atlaste/CPPCoverage/raw/c4b5ed0e39007295ea98e120b7f4b127681fe998/CoverageExt/Resources/Coverage-x64.exe
.\Coverage-x64.exe -p . -- npm.cmd test

Update 3

OpenCppCoverage works!

npm i -D npm
npm run rebuild -- -- --debug
OpenCppCoverage.exe --modules "*.node" --source "packages\bindings\src\*" -- node test.js

Time to format the data in a way that codecov likes and then package this up into the branch!

cinderblock · 2021-09-07T20:13:09Z

package.json

@@ -9,7 +9,10 @@
  "license": "MIT",
  "scripts": {
    "rebuild": "lerna run --stream rebuild",
-    "coverage": "nyc report --reporter=text-lcov > coverage.lcov && codecov",


nyc report --reporter=text-lcov > coverage.lcov is redundant with nyc --reporter lcovonly mocha (npm test).

cinderblock · 2021-09-12T06:23:25Z

@reconbot any thoughts?

cinderblock · 2021-09-12T06:24:37Z

.github/workflows/test.yml

@@ -1,19 +1,21 @@
 name: Test / Lint
 on:
  push:
+  pull_request:


The test-merge.yml workflow was basically identical to this one. I thought might as well de-dupe. Happy to not do this if you prefer.

yeah lets merge that in another pr and rebase this one

cinderblock · 2021-09-12T06:26:03Z

.github/workflows/test.yml

  test:
    runs-on: ${{ matrix.config.os }}
    strategy:
+      fail-fast: false


I say might as well let other tests finish even if there is an error. Cover more systems faster.

cinderblock · 2021-09-12T06:26:55Z

packages/bindings/binding.gyp

+        {
+          'cflags+': ['--coverage'],
+          'link_settings': {'libraries+': ['-lgcov']},
+          'xcode_settings': {


I've seen others put macOS conditions around xcode_settings but that doesn't seem to be necessary as far as I've seen.

cinderblock · 2021-09-12T06:28:37Z

packages/bindings/binding.gyp

+            'GCC_GENERATE_TEST_COVERAGE_FILES': ['YES'],
+            'GCC_INSTRUMENT_PROGRAM_FLOW_ARCS': ['YES'],
+            'OTHER_CFLAGS+': ['-fprofile-arcs -ftest-coverage'],
+            'OTHER_LDFLAGS+': [
+              '-fprofile-arcs -ftest-coverage',


I've tested without each setting individually. Theses all seem to be necessary instead of the one --coverage GCC flag.

Way to think different Xcode 😉

reconbot · 2021-09-12T12:45:59Z

I like it! I haven't had time to do a proper review

cinderblock · 2021-09-16T09:17:08Z

Well, it all seems to work! I bet some things could be cleaned up a bit but it all seems to be there.

cinderblock · 2021-10-06T18:56:53Z

@reconbot what are the steps for including this? I suspect this will be helpful for #2305 as well.

GazHank · 2021-10-06T21:39:13Z

Hi @cinderblock I agree and am super keen to expand the test coverage and visibility.

Sorry, other than a cursory glance at the coverage report output, I haven't had chance to take a closer look yet. I was a little surprised that the platform specific files showed different levels of coverage for similar functionality, but I guess some of this could be down to the different tooling per platform.

Overall I don't think this change should present much in the way of risk to any production users, and will help dev and test in the long term.

I know @reconbot is very busy at the moment, but am sure he will help to get this included when he gets some time.

cinderblock · 2021-10-06T21:54:17Z

No worries. I know how life can be. Just don't want this to fall by the wayside.

I would relish some thoughts on the new package.json#scripts. Getting native code coverage reports on Windows is significantly different than macOS/Linux. It might be nice to have a single script to run that does the right thing no matter the platform. However this very quickly would want to be some extra node script that effectively replaces what I've achieved already. I'm all for having as much code as possible in one language but this seems like diminishing returns.

Of note, I haven't tried running these tests with an Arduino (or equivalent) attached. That would almost certainly significantly affect coverage for various platforms.

It might be worth figuring out how to regularly run these tests with either a virtual (loopback) COM port (that's all the Arduino test code does anyway) or setting up a custom action runner just for this repo that has the dedicated hardware attached so that anyone can more easily run the full suite of tests.

Related, we might want to make the codecov token a proper secret. As it stands, anyone can add a report to codecov.

Notice I accidentally did this running tests on my personal machine:

reconbot · 2021-10-06T21:58:01Z

It might be worth figuring out how to regularly run these tests with either a virtual (loopback) COM port (that's all the Arduino test code does anyway) or setting up a custom action runner just for this repo that has the dedicated hardware attached so that anyone can more easily run the full suite of tests.

This has been my dream for years, we just need a loopback serialport.

reconbot · 2021-10-06T22:16:32Z

package.json

@@ -40,6 +46,7 @@
    "lerna": "^3.22.1",
    "mocha": "^8.3.0",
    "node-abi": "^3.0.0",
+    "npm": "^7.23.0",


Good question.This was because both of the Windows coverage tools I chose to use could not find npm.cmd on the path (or something like that).

I just started a branch to try without this explicitly: https://github.com/cinderblock/node-serialport/actions/runs/1314282300

You can see the Windows builds failing because:

[...] Error: Cannot find module 'D:\a\node-serialport\node-serialport\node_modules\npm\bin\npm-cli.js' [...]

We could probably solve this problem in an alternate way, but my initial goal was to get it working.

One simple alternative would be to add a specific npm install npm@7 step to the Windows workflows.

It works: https://github.com/cinderblock/node-serialport/actions/runs/1314333987

@reconbot would you prefer to not soil the devDependencies? I can merge the npm i npm step into this PR which fixes this problem. Just means that anyone that wants to run coverage tests on their Windows machines will need to install npm locally. That being said, all platforms need some extra tools installed for this all to work, so maybe this isn't a big deal.

Alternatively, I bet someone could fix the underlying problem with the Windows paths if they were feeling more ambitious than I am at the moment.

cinderblock · 2021-11-26T23:29:27Z

Some macOS updates broke the gcov linking that was previously done with a hard coded path. I put in a find based search for the needed libgcov.a file that I expect will be more reliable but still think a better solution could be found.

I also decided to include a change that splits the different OS tests up into fully separate jobs. This makes it a little more readable when following the jobs. We just need to remember to edit all 3 jobs for any test changes. Ultimately, I made this change because of loopback testing plans (#2327) and the assumption that each OS would have even more differences in their respective test environments.

reconbot · 2022-01-07T21:59:16Z

@cinderblock I've definitely messed up your pr with the introduction of #2384 I want to apologize for that. But I do hope it will a bunch easier to maintain the bindings in a separate repository. I'm game for helping move all this over there.

cinderblock · 2022-01-07T22:49:02Z

This makes sense! I think I'll have time to take a look at moving things over sometime this weekend.

Does this have implications for my other pending PRs too?

reconbot · 2022-03-05T04:44:05Z

This need to be moved to https://github.com/serialport/bindings-cpp

cinderblock added 6 commits September 6, 2021 12:50

Prettier (wants indentation)

582c4ac

Use the same workflow for all tests

774b0a5

Fail fast so that we can see all that goes wrong

1ec3783

Initial implementation of enabling coverage checks on C++ sources

e1408b9

Fix spelling and grammar

4e3f4a5

Think different for Xcode

e778699

cinderblock changed the title ~~WIP - Generate C++ coverage reports too!~~ Generate C++ coverage reports too! Sep 7, 2021

cinderblock added 4 commits September 6, 2021 17:06

Consistent punctuation

9e7204c

Dedupe and use environment variables

9e1b799

Clarify conditions by putting them first

0a0f696

Clarify comments

018739f

cinderblock mentioned this pull request Sep 7, 2021

Coverage for node modules OpenCppCoverage/OpenCppCoverage#158

Closed

cinderblock commented Sep 7, 2021

View reviewed changes

cinderblock mentioned this pull request Sep 10, 2021

Would this work with Node.js native modules? atlaste/CPPCoverage#35

Open

cinderblock commented Sep 12, 2021

View reviewed changes

Add Windows coverage

4f4d9e8

reconbot reviewed Oct 6, 2021

View reviewed changes

Try without npm dep

6a1ddde

cinderblock mentioned this pull request Oct 7, 2021

Tooling: Run full test suite with loopback serial device #2327

Open

21 tasks

Install npm manually on Windows

fd73eb2

cinderblock added 2 commits October 30, 2021 13:35

Split the test procedure up by OS

bc3466a

Fix macOS binding. A better solution should exist.

16a8a76

cinderblock added 9 commits November 26, 2021 15:40

Make sure we only ever get one directory

20b2f0f

Fix errors on non-macOS systems

094a85b

Cleaner implementation??

d76b437

Only on macOS

b1e425c

Indent

7f3d8c2

Cleanup steps

51c20a9

Force color on

471fc83

Run Windows specific coverage test in workflow

68fb9b3

Clean up and clarify new coverage procedures for different OSes

325a9a6

cinderblock mentioned this pull request Dec 4, 2021

CI cleanup #2363

Closed

Use global npm instead of extra install step

33bc240

reconbot closed this Mar 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate C++ coverage reports too! #2314

Generate C++ coverage reports too! #2314

cinderblock commented Sep 6, 2021 •

edited

Loading

cinderblock Sep 7, 2021

cinderblock commented Sep 12, 2021

cinderblock Sep 12, 2021

reconbot Oct 6, 2021

cinderblock Sep 12, 2021

cinderblock Sep 12, 2021

cinderblock Sep 12, 2021 •

edited

Loading

reconbot commented Sep 12, 2021

cinderblock commented Sep 16, 2021

cinderblock commented Oct 6, 2021

GazHank commented Oct 6, 2021

cinderblock commented Oct 6, 2021

reconbot commented Oct 6, 2021 •

edited

Loading

reconbot Oct 6, 2021

cinderblock Oct 7, 2021

cinderblock Oct 7, 2021 •

edited

Loading

cinderblock Oct 9, 2021 •

edited

Loading

cinderblock commented Nov 26, 2021 •

edited

Loading

reconbot commented Jan 7, 2022

cinderblock commented Jan 7, 2022

reconbot commented Mar 5, 2022

Generate C++ coverage reports too! #2314

Generate C++ coverage reports too! #2314

Conversation

cinderblock commented Sep 6, 2021 • edited Loading

Platforms & Issues

macOS

Update 1

Update 2

Windows

Update 1

Update 2

Update 3

Choose a reason for hiding this comment

cinderblock commented Sep 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cinderblock Sep 12, 2021 • edited Loading

Choose a reason for hiding this comment

reconbot commented Sep 12, 2021

cinderblock commented Sep 16, 2021

cinderblock commented Oct 6, 2021

GazHank commented Oct 6, 2021

cinderblock commented Oct 6, 2021

reconbot commented Oct 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cinderblock Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

cinderblock Oct 9, 2021 • edited Loading

Choose a reason for hiding this comment

cinderblock commented Nov 26, 2021 • edited Loading

reconbot commented Jan 7, 2022

cinderblock commented Jan 7, 2022

reconbot commented Mar 5, 2022

cinderblock commented Sep 6, 2021 •

edited

Loading

cinderblock Sep 12, 2021 •

edited

Loading

reconbot commented Oct 6, 2021 •

edited

Loading

cinderblock Oct 7, 2021 •

edited

Loading

cinderblock Oct 9, 2021 •

edited

Loading

cinderblock commented Nov 26, 2021 •

edited

Loading