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

Fix: Make rule-tester strictly check messageId. (ref #9890) #9908

Merged
merged 1 commit into from Mar 22, 2018

Conversation

Projects
None yet
4 participants
@betaorbust
Copy link
Contributor

betaorbust commented Jan 28, 2018

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

This PR:

  • Makes RuleTester compare messageId in the test to messageId in the runtime error.
  • Fixes an issue with tests/lib/rules/no-fallthrough.js where the wrong message and type data was being passed through.

What changes did you make? (Give an overview)
Currently, when RuleTester is used with a messageId property in the error section, the underlying implementation looks up the raw message string from rule.meta.messages[messageId] and compares it with the runtime error string produced by the rule.
All other properties checked in RuleTester's error section (message, type, line, column, endLine, endColumn) are direct comparisons to the resulting data, which makes the behavior of messageId an unexpected and confusing developer experience.

This PR addresses this by changing messageId checks in RuleTester to be direct comparisons.

Is there anything you'd like reviewers to focus on?
This partially addresses #9890, fixing the DX of messageId without dealing with the more contentious issue of surfacing data in the public API.

@platinumazure
Copy link
Member

platinumazure left a comment

Just one small message suggestion, otherwise this looks good to me. (N.B. I still need to check to make sure this does what we agreed on in the issue.)

assert.strictEqual(
error.messageId,
message.messageId,
`messageId mismatch saw '${message.messageId}' and expected '${error.messageId}'.`

This comment has been minimized.

@platinumazure

platinumazure Feb 3, 2018

Member

This message feels like a run-on sentence. I would suggest something like:

messageId '${message.messageId}' does not match expected messageId '${error.messageId}'.

This comment has been minimized.

@betaorbust

betaorbust Feb 25, 2018

Author Contributor

👍 Will do.

@betaorbust betaorbust changed the title Fix: Make rule-tester strictly check messageId. (ref #9890) WIP: Make rule-tester strictly check messageId. (ref #9890) Feb 25, 2018

@betaorbust betaorbust force-pushed the betaorbust:make-message-id-direct-compare--jf branch from 9ca36d8 to fa2ca54 Feb 25, 2018

@betaorbust betaorbust changed the title WIP: Make rule-tester strictly check messageId. (ref #9890) Fix: Make rule-tester strictly check messageId. (ref #9890) Feb 25, 2018

@betaorbust

This comment has been minimized.

Copy link
Contributor Author

betaorbust commented Feb 25, 2018

@platinumazure The code now aligns with what we discussed in #9890 (comment) and I think it's ready for the fix to go in.

@betaorbust betaorbust force-pushed the betaorbust:make-message-id-direct-compare--jf branch from fa2ca54 to 59437bf Feb 25, 2018

@betaorbust

This comment has been minimized.

Copy link
Contributor Author

betaorbust commented Feb 26, 2018

Looks like there’s some issues with older versions of node and assert.fail 😓
I’ll get a fix in.

Fix: Make rule-tester strictly check messageId. (ref #9890)
This change makes rule-tester check messageId directly by value provided instead
of the current behavior, which is to check the messageId's message value against
the message returned from the rule at runtime.

@betaorbust betaorbust force-pushed the betaorbust:make-message-id-direct-compare--jf branch from 59437bf to ff35ff8 Feb 26, 2018

@betaorbust

This comment has been minimized.

Copy link
Contributor Author

betaorbust commented Feb 26, 2018

Ok, all fixed up and ready for review.

@platinumazure
Copy link
Member

platinumazure left a comment

LGTM, but would like to wait for other team members to review. Thanks for contributing!

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Mar 22, 2018

Closing and reopening to make sure Travis/AppVeyor still pass with this change against latest master. Then I'll merge this in if all goes well.

@platinumazure platinumazure reopened this Mar 22, 2018

@platinumazure platinumazure merged commit 0bc4a38 into eslint:master Mar 22, 2018

5 checks passed

commit-message Commit message follows guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details

@eslint eslint bot locked and limited conversation to collaborators Sep 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.