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: actual messageId and expected messageId are switched in rule tester #11928

Merged
merged 1 commit into from Jul 4, 2019

Conversation

@mdjermanovic
Copy link
Member

commented Jul 1, 2019

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

[X] Other, please explain: Small fix in rule tester

ESLint Version: master
  
What did you do? Please include the actual source code causing the issue.

Add the following test example to tests/lib/rules/dot-location.js:

{
      code: "obj\n.property",
      output: "obj.\nproperty",
      options: ["object"],
      errors: [{ messageId: "expectedDotBeforeProperty" }]
}

What did you expect to happen?

AssertionError [ERR_ASSERTION]: messageId 'expectedDotAfterObject' does not match expected messageId 'expectedDotBeforeProperty'.
+ expected - actual

-expectedDotAfterObject
+expectedDotBeforeProperty

What actually happened? Please include the actual, raw output from ESLint.

AssertionError [ERR_ASSERTION]: messageId 'expectedDotAfterObject' does not match expected messageId 'expectedDotBeforeProperty'.
+ expected - actual

-expectedDotBeforeProperty
+expectedDotAfterObject

What changes did you make? (Give an overview)

Switched args in rule tester.

Is there anything you'd like reviewers to focus on?

@eslint eslint bot added the triage label Jul 1, 2019

@platinumazure
Copy link
Member

left a comment

LGTM, thanks! Nice catch!

@platinumazure platinumazure added accepted bug core and removed triage labels Jul 1, 2019

@kaicataldo
Copy link
Member

left a comment

LGTM. Thanks for contributing!

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Small nit: I think this could be called a bugfix rather than a chore since RuleTester is a public API/tool (albeit used by rule developers), but I don't feel strongly about changing the commit message. Maybe we could change the commit message on merge.

@mdjermanovic

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

Small nit: I think this could be called a bugfix rather than a chore since RuleTester is a public API/tool (albeit used by rule developers), but I don't feel strongly about changing the commit message. Maybe we could change the commit message on merge.

Sorry, my mistake, I read the guide about commit messages (Chore - ... anything that isn’t user-facing) and wrongly interpreted 'user' (like, someone who needs only 'User guide' menu from the website).

I'll change the message and PR title.

@mdjermanovic mdjermanovic force-pushed the mdjermanovic:msgidswitch branch from bca9c2a to 118b305 Jul 1, 2019

@mdjermanovic mdjermanovic changed the title Chore: Fix switched actual and expected messageIds in rule tester Fix: actual messageId and expected messageId are switched in rule tester Jul 1, 2019

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

No need to apologize, we get it wrong sometimes too :)

@not-an-aardvark not-an-aardvark merged commit 19335b8 into eslint:master Jul 4, 2019

9 checks passed

commit-message Commit message follows guidelines
Details
continuous-integration Build #20190701.2 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.