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
Fix: Remove duplicate error message on crash (fixes #8964) #10865
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -333,12 +333,28 @@ describe("bin/eslint.js", () => { | |
return Promise.all([exitCodeAssertion, outputAssertion]); | ||
}); | ||
|
||
it("prints the error message exactly once to stderr in the event of a crash", () => { | ||
const child = runESLint(["--rule=no-restricted-syntax:[error, 'Invalid Selector [[[']", "Makefile.js"]); | ||
const exitCodeAssertion = assertExitCode(child, 2); | ||
const outputAssertion = getOutput(child).then(output => { | ||
const expectedSubstring = "Syntax error in selector"; | ||
|
||
assert.strictEqual(output.stdout, ""); | ||
assert.include(output.stderr, expectedSubstring); | ||
|
||
// The message should appear exactly once in stderr | ||
assert.strictEqual(output.stderr.indexOf(expectedSubstring), output.stderr.lastIndexOf(expectedSubstring)); | ||
}); | ||
|
||
return Promise.all([exitCodeAssertion, outputAssertion]); | ||
}); | ||
|
||
it("prints the error message pointing to line of code", () => { | ||
const invalidConfig = `${__dirname}/../fixtures/bin/.eslintrc.yml`; | ||
const child = runESLint(["--no-ignore", invalidConfig]); | ||
const exitCodeAssertion = assertExitCode(child, 2); | ||
const outputAssertion = getOutput(child).then(output => { | ||
const expectedSubstring = "Error: bad indentation of a mapping entry at line"; | ||
const expectedSubstring = ": bad indentation of a mapping entry at line"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add an assertion here that the error message also contains the location of the invalid config file? e.g. assert.include(output.stderr, path.resolve(invalidConfig)); I vaguely remember that when I was looking at this, I was having trouble because the message "Cannot read config file: /path/to/config" wasn't appearing when I just printed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll add that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I added that line, the test fails. There is no mention of the filename in the stack. I'd suggest opening a separate issue to track that. Agree? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify, was the filename previously getting printed for you before this change? Currently, the output for me (on
To me, it seems like it's important for debugging that we tell users which config file has a problem when we report an "invalid config" error. So if the removal of In other words, my preference order for the possible outputs is:
So I'm fine with merging this PR if it's moving us from state (4) to state (3), but I have concerns about it if it's moving us from state (2) to state (3). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha, thanks for explaining. You are correct that (Side note: IIRC, this type of discrepancy is why I initially included both in the CLI. I actually don't think it's a problem to show duplicate error messages to get both the message and the stack, so I'm happy to just close the issue without merging this PR.) |
||
|
||
assert.strictEqual(output.stdout, ""); | ||
assert.include(output.stderr, expectedSubstring); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format of this error message was different on my machine. It said "YAMLException:" instead of "Error:". I split the difference and left the colon in.