Conversation
…k/exec. Minor fixups to tests to match new errors.
…s: coverage, speed.
| "headers": "node ./tools/header.js build/peggy.min.js build/benchmark-bundle.min.js build/test-bundle.min.js", | ||
| "deploy": "npm run deploy:peggy && npm run deploy:tests && npm run deploy:bench", | ||
| "coverage:bin": "PEGGY_CLI_DEBUG=1 npm run rollup && nyc --reporter lcov jest run.spec.ts", | ||
| "coverage:bin": "PEGGY_CLI_DEBUG=1 npm run rollup && jest run.spec.ts", |
There was a problem hiding this comment.
Make sure not to check in the sourceMap'd version, please.
There was a problem hiding this comment.
Sorry, I don't understand you :(
There was a problem hiding this comment.
jest doesn't follow fork/exec's when recording coverage. the nyc wrapper does. With this refactor, most of the tests don't need to fork/exec, so nyc isn't needed anymore.
|
|
||
| if (process.env.PEGGY_CLI_DEBUG) { | ||
| cli_config.output.sourcemap = true; | ||
| cli_config.output.sourcemap = "inline"; |
There was a problem hiding this comment.
jest can't handle external sourceMaps. Something to consider for our sourcemap output. We could, for example, make a filename of "inline" be magical, and the default.
There was a problem hiding this comment.
Maybe. Honestly, never used source maps, don't known how convenient it will be
There was a problem hiding this comment.
It's particularly convenient if people are using broken tools like jest. It will make more sense once you run npm run coverage:bin, and look at the end of the bin/peggy.js file.
Mingun
left a comment
Mingun
left a comment
There was a problem hiding this comment.
The first commit is very big and contains many different changes, so it was hard to review it. It would be better, if you make a separate commits for
- upgrade libraries
- move code from
peggy.mjstopeggy-cli.mjs - made a refactoring in several commits (one for callbacks -> Promise; one for changing options descriptions; ...)
- rewrite
peggy-cli.mjsas aCommand
But what is made, made.
Generally, two things need to be fixed:
- run tests on CI
- explicitly check
exitCodeand be sure that it is right in all cases. External utilities should have a clear indication of results of our CLI.
| "headers": "node ./tools/header.js build/peggy.min.js build/benchmark-bundle.min.js build/test-bundle.min.js", | ||
| "deploy": "npm run deploy:peggy && npm run deploy:tests && npm run deploy:bench", | ||
| "coverage:bin": "PEGGY_CLI_DEBUG=1 npm run rollup && nyc --reporter lcov jest run.spec.ts", | ||
| "coverage:bin": "PEGGY_CLI_DEBUG=1 npm run rollup && jest run.spec.ts", |
There was a problem hiding this comment.
Sorry, I don't understand you :(
bin/peggy-cli.mjs
Outdated
| inputStream.on("data", data => { input.push(data); }); | ||
| inputStream.on("end", () => resolve(Buffer.concat(input).toString())); | ||
| inputStream.on("error", er => { | ||
| Error.captureStackTrace(er); |
There was a problem hiding this comment.
Is that really necessary? If yes it would be worth to add a comment about the reasons
There was a problem hiding this comment.
There are a few places where errors coming into async callbacks don't have their stack traces filled in by default, at least on node 17.0.1. If you specify --verbose, the stack trace is printed. I've put comments on each one.
There was a problem hiding this comment.
According to the V8 docs and this SO answer, the stack captured by Error.captureStackTrace contains a stack of caller location. So it seems it useless to call it here because we in any case cannot add information about where the er is actually was constructed. Besides, I could not find information on whether the existing stack will be rewritten by this call, but if we want to leave it, then this needs to be avoided
There was a problem hiding this comment.
I thought it gave useful information when I tried it, but I'll just remove these calls.
| })).resolves.toBe(HELP); | ||
| await expect(exec({ | ||
| error: CommanderError, | ||
| errorCode: "commander.helpDisplayed", |
There was a problem hiding this comment.
I see that you added a default value for the exitCode property. I'd prefer to leave it explicitly defined to see that we actually check it in all cases. Moreover, it seems that in many cases the exit code changed from 1 to 0 which is incorrect:
- error when generating a parser should produce
1 - error when testing an input should produce
2 - otherwise it should produce
0
There was a problem hiding this comment.
I've put exitCode checks on all tests that expect CLI failure. I'm not sure what would have changed from 1 to 0? The only thing that produces 0 that I don't like is --help (which I want to return exit code 64), but I haven't figured out how to make commander do that.
|
|
||
| if (process.env.PEGGY_CLI_DEBUG) { | ||
| cli_config.output.sourcemap = true; | ||
| cli_config.output.sourcemap = "inline"; |
There was a problem hiding this comment.
Maybe. Honestly, never used source maps, don't known how convenient it will be
2 participants
This turned out to be a lot more work than expected, sorry it took so long.
Fixes #205