Fix broken test globs and coverage scripts #475
Conversation
|
I also considered updating mocha's version in this PR because it's 2 major versions behind latest. But I decided against it. However, the test suite did run SUCCESSFULLY 🎉 against the latest version (v7.1.2) without any further modifications! So presumably it would be safe to bump. |
It is not safe to use unquoted globstars within npm scripts because they are not expanded consistently on all versions of bash. Specifically, globstar isn't even available until version 4 of bash; and macOS still ships with v3. (Ergo, globstar won't work on stock macOS) Further, even if it _is_ available, it's only an option. Nothing ensures the option is enabled. Clearly, some prior developer recognized that the tests weren't all running and they "duplicated" the glob pattern to ensure it matched all files. However, they did 3 things wrong: 1. mocha (like virtually all *nix utilities) expects file arguments _last_ 2. they still didn't properly quote the globs, merely worked around the globstar issue by manually matching 3 directory levels 3. the workaround still doesn't robustly match recursive directories because it only goes three levels deep The proper fix is to quote the glob pattern such that mocha itself (by means of a dependency) will expand the globstar pattern consistently regardless of shell version.
Istanbul coverage is broken. Instead of fixing it, replace with nyc because istanbul is deprecated.
- `cover` script: - triggers test run (includes instrumentation) - prints report summary to STDOUT - generates html report for manual viewing - generates lcov report for coveralls - `coveralls` script: - triggers `cover` reporting script - uploads lcov.info report to coveralls
|
Updated the scripts to not run coverage as part of npm-test. To summarize, the istanbul coverage was broken prior to this PR. And after this PR, coverage is still broken, but is configured to use the istanbul cli replacement: nyc. nyc is reporting 0 coverage after this PR (which is an improvement from before this PR, where the coverage script actually errored). I haven't figured out why nyc is reporting 0 coverage, but I suspect it has something to do with the mocha --exit flag or requiring esm. (Or perhaps even the unhandled rejection hook.) Perhaps the fix for #487 will fix the coverage. Regardless, this PR doesn't leave the test suite or coverage in a worse place than before, but it still needs work. |
| "precover": "nyc npm test", | ||
| "cover": "nyc report -r lcov", | ||
| "precoveralls": "npm run cover", | ||
| "coveralls": "< coverage/lcov.info coveralls", |
There was a problem hiding this comment.
To clarify the weirdness of putting the file redirection in front: this allows extra flags to be passed when running via npm:
npm run coveralls -- --foo will pass --foo along to the coveralls command. If input/output redirection is tacked on at the end of the script as is common in shell, then that would cause a malformed command.
Summary
globs
It is not safe to use unquoted globstars within npm scripts because they
are not expanded consistently on all versions of bash. Specifically,
globstar isn't even available until version 4 of bash; and macOS still
ships with v3. (Ergo, globstar won't work on stock macOS)
Further, even if it is available, it's only an option. Nothing ensures
the option is enabled.
Clearly, some prior developer recognized that the tests weren't all
running and they "duplicated" the glob pattern to ensure it matched all
files. However, they did 3 things wrong:
last
globstar issue by manually matching 3 directory levels
because it only goes three levels deep
The proper fix is to quote the glob pattern such that mocha itself (by
means of a dependency) will expand the globstar pattern consistently
regardless of shell version.
istanbul
Istanbul coverage is broken (https://travis-ci.org/github/optimizely/javascript-sdk/jobs/683996183#L1612) but istanbul is also deprecated.
This replaces istanbul with nyc and cleans up the coverage scripts and flow. The coverage is still broken (needs some ES or TS plugins for nyc) but it's an improvement.
npm scripts
Also sorts the npm scripts a bit more logically and auto-runs lint after npm-test.