Adding additional configuration file options #436

mcknasty · 2022-12-04T21:38:15Z

Pull Request 436

Commit Message

commit 22a9c08
Author: The Nasty tmckeown@gmail.com
Date: Sat Jan 20 13:51:03 2024 -0500

Additional Configuration File Support (#436)

feat: Adding additional configuration file options
test: unit test for config file detection
test: unit test for config files pass via the cli
test: unit test case for early exit of hideInstrumenteeArgs
test: unit test for Node.js argument handling
fix: bug in spawning of c8 process that dropped coverage significantly

Following the Contributions Recommendations here.

Unit Tests Results

Test Type	PASS	Tests Passed	Tests Pending	Time
Initial Test	✔️	120 passing	17 pending	53 seconds
Snapshot Test	✔️	120 passing	17 pending	52 seconds
Final Test	✔️	120 passing	17 pending	54 seconds

Test completed in Node version v18.19.0

Node Version Testing Matrix

Node Version	PASS	Tests Passed	Tests Pending	Time
14.21.3	✔️	120 passing	17 pending	1 minutes
16.20.2	✔️	120 passing	17 pending	57 seconds
18.19.0	✔️	120 passing	17 pending	1 minutes
20.11.0	✔️	120 passing	17 pending	1 minutes

Duplicate of #447
References #448

edited 2022-12-05 4:58 AM EST. I noticed an error in my last patch, so I updated it.
edited 2023-02-20 12:41 PM EST. Updated pull request readme to the new format and linked existing issues.
edited 2023-07-28 11:50 AM EST. Adding additional unit tests for each of the config file options.
edited 2023-07-29 1:14 PM EST. Adding test case for loading invalid config files and updating readme to clarify ESM support.
edited 2024-01-08 3:17 PM EST. Fixing error that was causing code coverage drops. Changing Commit Message to include omitted items.
edited 2024-01-20 3:09 PM EST. Added test name for chia-snapshots. Skipped tests that require (feat: show derived configuration #517) to be merged. Updated an error handling test.

BREAKING CHANGE: switches to using NODE_V8_COVERAGE rather than inspector directly

@bcoe

Clarify project goals: I'd like this project to demonstrate a powerful code coverage solution relying primarily on native V8 functionality, and minimal user-land modules -- @bcoe

- issue template - PR template - contributing guide - maintainers guide closes bcoe#35

This commit fixes the conversion from `file://` urls to system-dependent paths. It ensures that it works on Windows and with special characters.

see: demurgos/v8-coverage#2

mcknasty · 2024-01-15T15:34:17Z

Overview

👏 The Good - ~~This Pull Request is getting close.~~ This PR is ready to go!
👌 The Bad - ~~Need to get~~ Another Pull Request merged ~~before this one is ready to go.~~ would get the rest of the unit tests ready. (#517)
👊 The Ugly - One of the Workflows is causing errors on ~~multiple~~ a pull request. (#453)

Update; There seems to be a dependency that has been updated that is solving some of these CD/CD concerns. Keeping the package-lock.json version number to 2 also seems to help for Node 14.

Workflow Errors

commit id checkout: 27d619c1b6005d0563098ab2d9909117586aa852
npm ci --engine-strict command failing

Run npm ci --engine-strict
  npm ci --engine-strict
  shell: /usr/bin/bash -e {0}
npm ERR! Cannot read property '@bcoe/v8-coverage' of undefined

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/2024-01-15T05_55_22_507Z-debug.log
Error: Process completed with exit code 1.

Attempt to Reproduce Locally

$ git remote -v
origin  git@github.com:mcknasty/c8.git (fetch)
origin  git@github.com:mcknasty/c8.git (push)
upstream        https://github.com/bcoe/c8.git (fetch)
upstream        https://github.com/bcoe/c8.git (push)
$ git branch | grep '*'
* additional-config-files
$ git log | head -n 1
commit 27d619c1b6005d0563098ab2d9909117586aa852
$ node -v
v14.21.3
$ npm -v
9.8.1
$ npm ci --engine-strict

added 383 packages, and audited 384 packages in 5s

98 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
$ uname --all
Linux XXXXXXXXX 5.15.133.1-microsoft-standard-WSL2 #1 SMP Thu Oct 5 21:02:42 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Is there a way to run the github actions locally? I initially tried a package called act. I didn't spend a ton of time with it, but it seemed to have some challenges. Are there any other packages I can try to reproduce this error or some additional information on act that could reproduce the errors in the workflow?

Updated 2024-01-20 at 12:49 PM EST. Added Reference to pull request for print config

test: absolute directories for tmpDir and reportsDir

test/parse-args-helper.js

feat: Adding additional configuration file options test: unit test for config file detection test: unit test for config files pass via the cli test: unit test case for early exit of hideInstrumenteeArgs test: unit test for Node.js argument handling

feat: Adding additional configuration file options test: unit test for config file detection test: unit test for config files pass via the cli test: unit test case for early exit of hideInstrumenteeArgs test: unit test for Node.js argument handling fix: bug in spawning of c8 process that dropped coverage significantly

mcknasty · 2024-01-20T23:53:48Z

Skipped the config detection tests for the moment. ~~Ready to go from my perspective.~~

kf6kjg · 2024-01-21T05:12:12Z

lib/parse-args.js

+
+  // Should the process die if none of these filename extensions are found?
+  if (Object.keys(config).length === 0) {
+    throw new Error(`Unsupported file type ${ext} while reading file ${path}`)


I read why this was added, but it's going to result in bad UX when a config file return an empty object - in that situation it loaded fine, and is not an invalid file type, it's just not got any config entries.

I want to be sure I am clear on what you stated above. To rewrite what you expressed in two use cases.

Scenario:

A user executes the c8 command with the --config parameter set to an existing file on the file system

Given:

The file has a valid file name as documented in the c8 readme
The file has no contents.

Result:
c8 will run with a default configuration

Scenario:

A user executes the c8 command without additional configuration arguments and c8 discovers a configuration file.

Given:
The file has a valid file name as documented in the c8 readme
The file has no contents.

Result:
c8 will run with a default configuration

Can you please confirm?

The block does cover the following use case.

Scenario:
A user executes the c8 command with the --config parameter set to an existing file on the file system.

Given:
The file does not have a valid file name as documented in the c8 readme

Result:
c8 will produce the error `Unsupported file type ...'

This test case can be found on Line 70 - Line 85 of the file test/parse-args.js

Some notes before I hit the scenarios:

Only YAML would have an empty file return an empty object.

An empty JSON file is a syntax error.

A JSON file of {} is valid.

A CJS script that doesn't export anything IMO SHOULD result in an error as this is a common mistake.

A CJS script that exports an empty object, e.g. {} is valid.

So, scenarios:

Scenario:
A user executes the c8 command with the --config parameter set to an existing file on the file system
OR
A user executes the c8 command without the --config parameter set AND c8 discovers a configuration file.

Given any of:

The file is JSON with the .json extension, case insensitive.
The file has the contents {} or equivalent.

The file is YAML with the .yaml or .yml extension, case insensitive.
The file has no contents

The file is CommonJS with the .js or .cjs extension, case insensitive.
The file's contents result in an empty object {} being exported.

Result:
c8 will run with a default configuration.

Scenario:
A user executes the c8 command with the --config parameter set to an existing file on the file system
OR
A user executes the c8 command without the --config parameter set AND c8 discovers a configuration file.

Given any of:

The file has the .json extension, case insensitive.
The file is invalid JSON.

The file has the .yaml or .yml extension, case insensitive.
The file is invalid YAML.

The file has the .js or .cjs extension, case insensitive.
The file's contents are invalid CommonJS or have a runtime error.

The file has the .js or .cjs extension, case insensitive.
The file doesn't export anything.

The file has the .js or .cjs extension, case insensitive.
The file exports something that's not an object.

Result:
c8 will fail with an appropriate error based on the actual underlying problem that will be helpful to the user.

Some errors are really very obtuse or otherwise mask the real problem. I think over time of people testing this those errors will be discovered and made nicer, so making the errors nicer probably shouldn't be part of this PR.

Scenario:
A user executes the c8 command with the --config parameter set to an existing file on the file system.

Given:
The file name is NOT one of the ones documented in the c8 readme.
The file extension is one of the valid types, e.g. .js, .cjs, .json, .yaml, .yml etc.
The file returns a valid c8 configuration.

Result:
c8 will run with the configuration coming from the file given.

This is because there are several reasons for using the --config parameter, here's just a few:

I've got multiple config files to choose from. This you've covered.

My config file's at an unusual path that auto detection won't find, e.g. .configs/c8rc.yaml This I'm pretty sure you've covered, though maybe not directly with a test.

I don't like any of the default file names and have my own naming scheme I use, e.g. .config.c8.yaml - the current code looks like it handles this case already, there just doesn't seem to be a test covering this idea.

I've created a PR for the branch of this PR that adds tests and changes to support the above.

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

lib/parse-args.js

+  if (jsExts.includes(ext)) {
+    config = require(path)
+  } else if (ymlExts.includes(ext)) {
+    config = require('js-yaml').load(readFileSync(path, 'utf8'))
+  } else if (ext === '.json' || fileName.slice(-2) === 'rc') {
+    config = JSON.parse(readFileSync(path, 'utf8'))
+  }


I am going to put this into a try/catch block for the following reasons ...

If a not properly formated json or yaml object is returned, it will produce an error.

If the path doesn't exist, it will produce an error.

If an unsupported configuration value is present in the configuration file, the value will get ignored.

It could be helpful to have a config schema validator. Going to add unit tests for two of the potential issues above.

I agree on a schema validator. It would help people a lot.

However masking erroneous configuration files by just ignoring them, or even just logging the problem and proceeding with defaults, is an antipattern to me. It results in unexpected behavior since c8 proceeds with different settings than intended.

In my PR to this PR I did wrap some of these in try-catch, but I only caught specific errors, re-thowing the others. The idea was to enhance the message in those cases as the underlying error was obtuse. However I think that other errors, such as file not found, are generally acceptable to leave alone.

JSON.parse is very well known for producing REALLY opaque errors. Unexpected end of JSON input always got me.

test/parse-args-helper.js

feat: Adding additional configuration file options test: unit test for config file detection test: unit test for config files pass via the cli test: unit test case for early exit of hideInstrumenteeArgs test: unit test for Node.js argument handling fix: bug in spawning of c8 process that dropped coverage significantly

kf6kjg · 2024-02-02T14:44:00Z

Wait what, why closed?😳

mcknasty · 2024-02-02T14:55:37Z

sorry was editing my name in the commit history and it closed the branch. I'll fix it.

feat: Adding additional configuration file options test: unit test for config file detection test: unit test for config files pass via the cli test: unit test case for early exit of hideInstrumenteeArgs test: unit test for Node.js argument handling fix: bug in spawning of c8 process that dropped coverage significantly

bcoe and others added 30 commits December 17, 2017 12:07

test: add fibonacci example (#10)

ab6dcbe

chore(release): 2.0.0

a3adc37

chore: add support for subprocess spawning (#11)

d2e959a

feat: switch to using Node's built in coverage (bcoe#22)

871b42c

BREAKING CHANGE: switches to using NODE_V8_COVERAGE rather than inspector directly

chore: configure Travis/Coveralls (bcoe#23)

cd2fbbf

chore(release): 3.0.0

3dd6ec4

docs: amend CHANGELOG

4a9c626

chore(release): 3.0.1

5d43d26

docs: slight tweak to README

bd60dd8

chore(release): 3.0.2

c6929c7

chore: add git repo

bd888d9

chore(release): 3.0.3

70ef42c

feat: allow relative paths to be optionally included

6339a1a

chore(release): 3.1.0

a2b20f1

fix: make tests run on Windows (bcoe#25)

39ebad6

feat: improve test assertions (bcoe#28)

277c9d0

feat: warn instead of throw on exception (bcoe#29)

e95c77b

chore(release): 3.2.0

cc2560b

docs: add clarification about goals of this project (bcoe#33)

0c7a497

Clarify project goals: I'd like this project to demonstrate a powerful code coverage solution relying primarily on native V8 functionality, and minimal user-land modules -- @bcoe

chore(maintenance): add project guides (bcoe#36)

9401072

- issue template - PR template - contributing guide - maintainers guide closes bcoe#35

chore(bin): rewrite to async I/O (bcoe#32)

f2818de

fix: process coverage merging (bcoe#37)

2ab914e

fix: address file:// issue with CJS (bcoe#39)

33fc5aa

chore(release): 3.2.1

7e46aeb

feat: allow script wrapper length to be specified (bcoe#51)

604ce3a

fix: file URL to system path conversion (bcoe#46)

6ab02d6

This commit fixes the conversion from `file://` urls to system-dependent paths. It ensures that it works on Windows and with special characters.

fix: snapshot

687f0c2

fix: float patch for branch/function coverage merge bug (bcoe#56)

d3f6ddd

see: demurgos/v8-coverage#2

chore: enable windows tests (bcoe#57)

c88b1f0

feat: add thresholds for enforcing coverage percentage (bcoe#59)

e2c2283

mcknasty force-pushed the additional-config-files branch from a89e0c7 to 27d619c Compare January 15, 2024 05:55

test: relative directories for tmpDir and reportsDir

2ee581c

test: absolute directories for tmpDir and reportsDir

mcknasty force-pushed the additional-config-files branch from 34900ce to d26cfbd Compare January 20, 2024 06:31

mcknasty commented Jan 20, 2024

View reviewed changes

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

mcknasty force-pushed the additional-config-files branch from d26cfbd to 2c5f1b2 Compare January 20, 2024 08:08

mcknasty force-pushed the additional-config-files branch from 2c5f1b2 to 43c3a45 Compare January 20, 2024 08:23

mcknasty force-pushed the additional-config-files branch 2 times, most recently from 0897804 to a534f68 Compare January 20, 2024 08:47

mcknasty force-pushed the additional-config-files branch from a534f68 to 22a9c08 Compare January 20, 2024 18:51

kf6kjg reviewed Jan 21, 2024

View reviewed changes

mcknasty commented Jan 21, 2024

View reviewed changes

mcknasty commented Jan 22, 2024

View reviewed changes

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

kf6kjg mentioned this pull request Jan 25, 2024

Enhancing tests of config parse mcknasty/c8#5

Closed

3 tasks

mcknasty force-pushed the additional-config-files branch from d8681e2 to 22a9c08 Compare February 1, 2024 21:23

mcknasty force-pushed the additional-config-files branch from 122be90 to 570ca76 Compare February 1, 2024 21:37

mcknasty closed this Feb 2, 2024

mcknasty force-pushed the additional-config-files branch from 570ca76 to 51d420a Compare February 2, 2024 14:40

This was referenced Feb 2, 2024

Additional Configuration File Support #518

Draft

Load config mcknasty/c8#7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding additional configuration file options #436

Adding additional configuration file options #436

mcknasty commented Dec 4, 2022 •

edited

mcknasty commented Jan 15, 2024 •

edited

mcknasty commented Jan 20, 2024 •

edited

kf6kjg Jan 21, 2024

mcknasty Jan 21, 2024 •

edited

kf6kjg Jan 22, 2024

kf6kjg Jan 25, 2024

mcknasty Jan 21, 2024 •

edited

kf6kjg Jan 22, 2024

kf6kjg Jan 25, 2024 •

edited

kf6kjg commented Feb 2, 2024

mcknasty commented Feb 2, 2024

Adding additional configuration file options #436

Adding additional configuration file options #436

Conversation

mcknasty commented Dec 4, 2022 • edited

Pull Request 436

Commit Message

Following the Contributions Recommendations here.

Unit Tests Results

Node Version Testing Matrix

mcknasty commented Jan 15, 2024 • edited

Overview

Workflow Errors

Attempt to Reproduce Locally

mcknasty commented Jan 20, 2024 • edited

kf6kjg Jan 21, 2024

Choose a reason for hiding this comment

mcknasty Jan 21, 2024 • edited

Choose a reason for hiding this comment

kf6kjg Jan 22, 2024

Choose a reason for hiding this comment

kf6kjg Jan 25, 2024

Choose a reason for hiding this comment

mcknasty Jan 21, 2024 • edited

Choose a reason for hiding this comment

kf6kjg Jan 22, 2024

Choose a reason for hiding this comment

kf6kjg Jan 25, 2024 • edited

Choose a reason for hiding this comment

kf6kjg commented Feb 2, 2024

mcknasty commented Feb 2, 2024

mcknasty commented Dec 4, 2022 •

edited

mcknasty commented Jan 15, 2024 •

edited

mcknasty commented Jan 20, 2024 •

edited

mcknasty Jan 21, 2024 •

edited

mcknasty Jan 21, 2024 •

edited

kf6kjg Jan 25, 2024 •

edited