Increase Code Quality By Increasing Code Coverage: lib/parse-args.js #447

mcknasty · 2023-01-17T13:51:15Z

Pull Request 447

Increase code quality by increasing code coverage: lib/parse-args.js

refactor: bumping code coverage to 100% for lib/parse-args.js. (#447)
test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (#447)
test: Early exit branch test for hideInstrumenteeArgs function. (#447)
test: Adding test for relative paths for relative report directory cli option. (#447)
test: Adding test case for relative paths for temporary directory cli option. (#447)
test: Adding test for passing node.js arguments to c8. (#447)

Following the Contributions Recommendations here.

File Code Coverage Matrix Report

Test Type	File	Statement	Branch	Function	Lines
Final Test	lib/parse-args.js	100%	100%	100%	100%
Node 14.21.3	lib/parse-args.js	100%	100%	100%	100%
Node 16.20.2	lib/parse-args.js	100%	100%	100%	100%
Node 18.19.0	lib/parse-args.js	100%	100%	100%	100%
Node 20.11.0	lib/parse-args.js	100%	100%	100%	100%

Unit Tests Results

All tests run on Node version 18.19.0

Test Type	PASS	Tests Passed	Tests Failed	Time
Initial Test	✔️	100 passing	0 failures	48 seconds
Snapshot Test	✔️	100 passing	0 failures	48 seconds
Final Test	✔️	100 passing	0 failures	48 seconds

Node Version Testing Matrix

Node Version	PASS	Tests Passed	Tests Failed	Time
14.21.3	✔️	100 passing	0 failures	55 seconds
16.20.2	✔️	100 passing	0 failures	1 minutes
18.19.0	✔️	100 passing	0 failures	55 seconds
20.11.0	✔️	100 passing	0 failures	1 minutes

Referencing Issue #448

Updated 2023-07-29 7:59 PM EST
Updated 2023-07-30 11:25 AM EST
Updated 2024-01-14 9:03 PM EST

bcoe

Thanks for the contribution, I'm excited to improve the coverage of c8.

In this PR specifically, I don't like that it changes the behaviour in subtle ways:

default will no longer give a hint in the UI that shows that we've defaulted to NODE_V8_COVERAGE.
The path of tempDirectory will no longer be an absolute path (./tmp, vs., /full/path/to/tmp).

I think you should be able to get coverage up without making these changes, one option would be just adding some ignore lines if necessary (since the logic is part of the setup of yargs, and not really part of the application logic).

…e#447) Test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447)

…e#447) test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447)

lib/parse-args.js

…e#447) test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447) test: Ealry exit branch test for hideInstrumenteeArgs function. (bcoe#447) test: Adding test for relative paths for relative report directories. (bcoe#447)

lib/parse-args.js

refactor: bumping code coverage to 100% for lib/parse-args.js. (bcoe#447) test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447) test: Ealry exit branch test for hideInstrumenteeArgs function. (bcoe#447) test: Adding test for relative paths for relative report directory cli option. (bcoe#447) test: Adding test case for relative paths for temporary directory cli option. (bcoe#447) test: Adding test for passing node.js arguments to c8. (bcoe#447)

test/parse-args.js

lib/parse-args.js

bcoe

Left a couple nits, I'd rather avoid changing behaviour around path resolution. The reason being that people have complicated setups with c8, where they run it in different directories, run it against output from web browsers (which may have files within them that are not relative to the director that c8 is running in, etc).

tldr; it might be okay to resolve the reportsDir/tmpDir, but it may have unintended consequences upstream.

Perhaps I'm being too cautious, but I'd rather not change this part of the library (unless we're trying to address outstanding bugs, and have a regression test).

mcknasty · 2023-07-29T20:52:45Z

The issue of the check function mentioned above has come up a few times as I have been working on yargs related feature requests and code coverage reports. I outlined the issue as I saw it in #489. Furthermore, merging pull request #436 could change the delta of this pull request a fair amount. I suggest that I outline the various ways of setting the temp directory and then write unit tests against those scenarios. I think we can revisit this issue once these few dependencies are complete.

@bcoe

refactor: bumping code coverage to 100% for lib/parse-args.js. (bcoe#447) test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447) test: Ealry exit branch test for hideInstrumenteeArgs function. (bcoe#447) test: Adding test for relative paths for relative report directory cli option. (bcoe#447) test: Adding test case for relative paths for temporary directory cli option. (bcoe#447) test: Adding test for passing node.js arguments to c8. (bcoe#447)

refactor: bumping code coverage to 100% for lib/parse-args.js. (bcoe#447) test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447) test: Early exit branch test for hideInstrumenteeArgs function. (bcoe#447) test: Adding test for relative paths for relative report directory cli option. (bcoe#447) test: Adding test case for relative paths for temporary directory cli option. (bcoe#447) test: Adding test for passing node.js arguments to c8. (bcoe#447)

refactor: bumping code coverage to 100% for lib/parse-args.js. (bcoe#447) test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447) test: Early exit branch test for hideInstrumenteeArgs function. (bcoe#447) test: Adding test for relative paths for relative report directory cli option. (bcoe#447) test: Adding test case for relative paths for temporary directory cli option. (bcoe#447) test: Adding test for passing node.js arguments to c8. (bcoe#447) test: relative directories for tempDirectoy and reportDir

refactor: bumping code coverage to 100% for lib/parse-args.js. (bcoe#447) test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447) test: Early exit branch test for hideInstrumenteeArgs function. (bcoe#447) test: Adding test for relative paths for relative report directory cli option. (bcoe#447) test: Adding test case for relative paths for temporary directory cli option. (bcoe#447) test: Adding test for passing node.js arguments to c8. (bcoe#447)

refactor: removing / testing lines of code for lib/parse-args.js to increase coverage. (bcoe#447) test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447) test: Early exit branch test for hideInstrumenteeArgs function. (bcoe#447) test: Adding test for relative paths for relative report directory cli option. (bcoe#447) test: Adding test case for relative paths for temporary directory cli option. (bcoe#447) test: Adding test for passing node.js arguments to c8. (bcoe#447)

mcknasty · 2024-01-20T17:36:49Z

test/parse-args.js

+    /** Commenting out till the print config PR is ready * /
+    it('should set it if undefined', () => {
+      const NODE_V8_COVERAGE = process.env.NODE_V8_COVERAGE
+      process.env.NODE_V8_COVERAGE = undefined
+      process.argv = ['node', 'c8', '--foo=99', 'my-app', '--help']
+      const argv = buildYargs().parse(hideInstrumenteeArgs())
+      argv.tempDirectory.endsWith('/coverage/tmp').should.be.equal(true)
+      delete process.env.NODE_V8_COVERAGE
      process.env.NODE_V8_COVERAGE = NODE_V8_COVERAGE
    })
+    /** */


Another candidate for updating unit tests as it relates to (#516)

mcknasty · 2024-01-21T23:50:10Z

test/parse-args.js

+    /** Commenting out till the print config PR is ready * /
+    it('should set it if undefined', () => {
+      const NODE_V8_COVERAGE = process.env.NODE_V8_COVERAGE
+      process.env.NODE_V8_COVERAGE = undefined
+      process.argv = ['node', 'c8', '--foo=99', 'my-app', '--help']
+      const argv = buildYargs().parse(hideInstrumenteeArgs())
+      argv.tempDirectory.endsWith('/coverage/tmp').should.be.equal(true)
+      delete process.env.NODE_V8_COVERAGE
      process.env.NODE_V8_COVERAGE = NODE_V8_COVERAGE
    })
+    /** */


Need to uncomment and skip this test.

refactor: removing / testing lines of code for lib/parse-args.js to increase coverage. (bcoe#447) test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447) test: Early exit branch test for hideInstrumenteeArgs function. (bcoe#447) test: Adding test for relative paths for relative report directory cli option. (bcoe#447) test: Adding test case for relative paths for temporary directory cli option. (bcoe#447) test: Adding test for passing node.js arguments to c8. (bcoe#447)

mcknasty force-pushed the parse-args-code-coverage branch from 8978fe1 to 175d031 Compare January 17, 2023 16:06

mcknasty changed the title ~~Parse args code coverage~~ lib/parse-args.js improved code coverage Jan 17, 2023

mcknasty mentioned this pull request Jan 17, 2023

Improve code quality by increasing code coverage. #448

Open

bcoe requested changes Jan 23, 2023

View reviewed changes

mcknasty added a commit to mcknasty/c8 that referenced this pull request Jan 28, 2023

refactor: Increasing code coverage of lib/parse-args.js to 100%. (bco…

ba0acbc

…e#447) Test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447)

mcknasty force-pushed the parse-args-code-coverage branch from 175d031 to ba0acbc Compare January 28, 2023 07:09

This comment was marked as outdated.

Sign in to view

mcknasty added a commit to mcknasty/c8 that referenced this pull request Jan 28, 2023

refactor: Increasing code coverage of lib/parse-args.js to 100%. (bco…

fbe5d72

…e#447) test: Adding a test case for NODE_V8_COVERAGE and changing the describe block title. (bcoe#447)

mcknasty force-pushed the parse-args-code-coverage branch from 704bce3 to fbe5d72 Compare January 28, 2023 22:57

mcknasty commented Jan 28, 2023

View reviewed changes

lib/parse-args.js Outdated Show resolved Hide resolved

mcknasty force-pushed the parse-args-code-coverage branch from fbe5d72 to 0eef52a Compare January 29, 2023 22:46

mcknasty commented Jan 29, 2023

View reviewed changes

lib/parse-args.js Show resolved Hide resolved

mcknasty commented Jan 30, 2023

View reviewed changes

lib/parse-args.js Outdated Show resolved Hide resolved

mcknasty force-pushed the parse-args-code-coverage branch from 0eef52a to 692ec75 Compare February 1, 2023 22:24

mcknasty requested a review from bcoe February 1, 2023 23:47

mcknasty changed the title ~~lib/parse-args.js improved code coverage~~ Increase Code Quality By Increasing Code Coverage: lib/parse-args.js improved code coverage Feb 4, 2023

mcknasty changed the title ~~Increase Code Quality By Increasing Code Coverage: lib/parse-args.js improved code coverage~~ Increase Code Quality By Increasing Code Coverage: lib/parse-args.js Feb 4, 2023

mcknasty mentioned this pull request Feb 10, 2023

Improve code quality by increasing code coverage: lib/source-map-from-file.js #453

Closed

12 tasks

bcoe reviewed Feb 14, 2023

View reviewed changes

test/parse-args.js Outdated Show resolved Hide resolved

bcoe reviewed Feb 14, 2023

View reviewed changes

lib/parse-args.js Outdated Show resolved Hide resolved

bcoe requested changes Feb 14, 2023

View reviewed changes

mcknasty mentioned this pull request Feb 20, 2023

Adding additional configuration file options #436

Closed

12 tasks

mcknasty force-pushed the parse-args-code-coverage branch from 692ec75 to 2c7bd7e Compare July 29, 2023 21:09

mcknasty force-pushed the parse-args-code-coverage branch from 2c7bd7e to 909e209 Compare July 30, 2023 00:09

mcknasty force-pushed the parse-args-code-coverage branch from 909e209 to 04b0e61 Compare July 30, 2023 14:33

mcknasty force-pushed the parse-args-code-coverage branch from 04b0e61 to 80fe2ac Compare July 30, 2023 14:39

mcknasty marked this pull request as draft July 30, 2023 16:16

mcknasty mentioned this pull request Jul 30, 2023

test: additional tests for relative path support #490

Merged

12 tasks

mcknasty force-pushed the parse-args-code-coverage branch from 80fe2ac to eada810 Compare January 14, 2024 23:35

mcknasty force-pushed the parse-args-code-coverage branch from eada810 to 977cead Compare January 14, 2024 23:47

mcknasty force-pushed the parse-args-code-coverage branch from 977cead to 5ca85f9 Compare January 15, 2024 00:14

mcknasty force-pushed the parse-args-code-coverage branch from 5ca85f9 to 7e61196 Compare January 15, 2024 02:05

mcknasty force-pushed the parse-args-code-coverage branch from 7e61196 to 29d3e1a Compare January 15, 2024 03:23

mcknasty force-pushed the parse-args-code-coverage branch from 29d3e1a to ba84f60 Compare January 15, 2024 05:16

mcknasty marked this pull request as ready for review January 15, 2024 05:21

mcknasty requested a review from bcoe January 15, 2024 05:21

mcknasty force-pushed the parse-args-code-coverage branch from 92d9ab4 to 2c5f1b2 Compare January 20, 2024 09:48

mcknasty force-pushed the parse-args-code-coverage branch from 2c5f1b2 to dcd63a5 Compare January 20, 2024 17:33

mcknasty commented Jan 20, 2024

View reviewed changes

mcknasty commented Jan 21, 2024

View reviewed changes

mcknasty marked this pull request as draft January 22, 2024 00:17

bcoe closed this Jun 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase Code Quality By Increasing Code Coverage: lib/parse-args.js #447

Increase Code Quality By Increasing Code Coverage: lib/parse-args.js #447

mcknasty commented Jan 17, 2023 •

edited

Loading

bcoe left a comment

This comment was marked as outdated.

bcoe left a comment

mcknasty commented Jul 29, 2023 •

edited

Loading

mcknasty Jan 20, 2024

mcknasty Jan 21, 2024

Increase Code Quality By Increasing Code Coverage: lib/parse-args.js #447

Increase Code Quality By Increasing Code Coverage: lib/parse-args.js #447

Conversation

mcknasty commented Jan 17, 2023 • edited Loading

Pull Request 447

Following the Contributions Recommendations here.

File Code Coverage Matrix Report

Unit Tests Results

Node Version Testing Matrix

bcoe left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

bcoe left a comment

Choose a reason for hiding this comment

mcknasty commented Jul 29, 2023 • edited Loading

mcknasty Jan 20, 2024

Choose a reason for hiding this comment

mcknasty Jan 21, 2024

Choose a reason for hiding this comment

mcknasty commented Jan 17, 2023 •

edited

Loading

mcknasty commented Jul 29, 2023 •

edited

Loading