Load config #7
Load config #7
Conversation
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.
|
The dreaded can find the package error message on node 14 on mac, windows, and linux. I don't think this is one were are able to fix. |
|
ref (#5 ), PR #436 on upstream, PR #518 on upstream |
|
@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. |
|
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. |
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
force-pushed
the
load-config
branch
3 times, most recently
from
d3738dc
to
e034e73
Compare
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
|
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: |
| 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') |
There was a problem hiding this comment.
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.
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.
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. |
|
Tests being in the same folder using the same base name as the files they test is my normal pattern in my own code. |
Checklist
npm test, tests passingnpm run test:snap(to update the snapshot)