Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load config #7

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Load config #7

wants to merge 6 commits into from

Conversation

mcknasty
Copy link
Owner

@mcknasty mcknasty commented Feb 2, 2024

Checklist
  • npm test, tests passing
  • npm run test:snap (to update the snapshot)
  • tests and/or benchmarks are included
  • documentation is changed or added

mcknasty and others added 3 commits February 2, 2024 10:16
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
Logically these are not part of the argument parsing. They are configuration loading and validation. Having them in the parse-args module was only making the file and the tests harder to work with.

This change only has two semantic differences:
1. The list of known config files names was converted to a const immutable array instead of beating a mutable array returned by a function.
2. The parse-args file is no longer exporting those two internal utility functions, instead the equivalents are coming from a dedicated file.
Specifically:
- Handling the cases of empty files and files that have simple forms of improper content.
- Reporting easier to understand errors when the parsers fail.

Adding schema validators was deemed beyond the scope of this particular commit, though notes were added for where they should be added.

Incorporated many of mcknasty's suggestions. However I didn't keep them all, and many other elements were reinterpreted.
@mcknasty
Copy link
Owner Author

mcknasty commented Feb 2, 2024

The dreaded can find the package error message on node 14 on mac, windows, and linux.

npm ERR! Cannot read property '@bcoe/v8-coverage' of undefined

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/distiller/.npm/_logs/2024-02-02T18_48_23_141Z-debug.log

Exited with code exit status 1

I don't think this is one were are able to fix.

@mcknasty
Copy link
Owner Author

mcknasty commented Feb 2, 2024

ref (#5 ), PR #436 on upstream, PR #518 on upstream

@mcknasty
Copy link
Owner Author

mcknasty commented Feb 2, 2024

@kf6kjg Please use this branch moving forward.

The only item we had left to discuss was the poorman's dependency injection

I think I handled the rest during the merging. Please be sure to compare the two branches. If you need help merging anything please let me know.

@kf6kjg
Copy link
Collaborator

kf6kjg commented Feb 2, 2024

Oof. I just spent the last several hours carefully reworking and creating #8 - and locally I've no issues with NPM 6 / Node 14 in that branch.

Happy to continue the discussion.

@mcknasty
Copy link
Owner Author

mcknasty commented Feb 2, 2024

Oof. I just spent the last several hours carefully reworking and creating #8 - and locally I've no issues with NPM 6 / Node 14 in that branch.

Happy to continue the discussion.

@kf6kjg

The Node 14 issue is something upstream. I saw it multiple times a few weeks ago. It didn't want to trigger on your PR for whatever reason. I just toggled a setting on circleci. If you push an update it might trigger.

@mcknasty mcknasty force-pushed the load-config branch 3 times, most recently from d3738dc to e034e73 Compare February 3, 2024 01:48
refactor: var names and assignment in helpers
refactor: removed some lines in helpers
refactor: test/load-config.js to use only expect style syntax
fix: removing path from dispaying on unit tests
refactor: test case for config file detection
refactor: removed dep on chai should
fix: added a describe block to the error handling tests
fix: unit test that was failing on ci due to difference in plaform json packages
@kf6kjg
Copy link
Collaborator

kf6kjg commented Feb 5, 2024

It's your PR to make as you wish, but I'd just like to note that I purposefully wrap all my tests in a describe block that describes which file the tests are executing from. Here's a diagram I made that describes the pattern I follow for test suite architecture:
image

it('throws an error if the JSON file is invalid JSON', () => {
const errorPattern = 'Error loading configuration from file ".+": ' +
'must contain a valid c8 configuration object. Original error: .*'
const errorRegEx = new RegExp(errorPattern, 'g')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has made this test too specific, it's now testing not just the loadConfigFile function, but redundantly also the implementation of ConfigParsingError - which has its own tests. That was originally done in my code because ConfigParsingError was not part of the exposed API. But now that it is in its own file and has its own tests, we only need a simple check for the fact that the loadConfigFile function used the ConfigParsingError correctly, e.g. that it includes the string Unexpected token in the message.
image

@mcknasty
Copy link
Owner Author

mcknasty commented Feb 6, 2024

It's your PR to make as you wish, but I'd just like to note that I purposefully wrap all my tests in a describe block that describes which file the tests are executing from. Here's a diagram I made that describes the pattern I follow for test suite architecture:

Thank you for this diagram. I will look into it a bit further next week. Due to potential security concerns, I removed the file name string displayed in the test results. It exposes an absolute path on the file system, and we should be able to exchange build logs to debug test cases. Furthermore, if we decide to run the test harness through a snapshot, it will fail due to the variations in development environments.

I have seen JavaScript development patterns where the module file and test cases are put in the same folder. For this particular project, I just run the following command from the project's directory root, if there is a specific test case I am looking for.

grep -Rni 'text in describe string' test/*

I am sure you can do something similar in most integrated development environments.

Additionally, we could mention in the jsdoc blocks where the unit tests are, or even at the top of the file.

@kf6kjg
Copy link
Collaborator

kf6kjg commented Feb 8, 2024

Tests being in the same folder using the same base name as the files they test is my normal pattern in my own code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants