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

test: fix test-suite to work with node master #9688

Merged

Conversation

MylesBorins
Copy link
Contributor

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:

What changes did you make? (Give an overview)

Changes to the output of strictEqual are breaking the test suite
This minor adjustment to the regex will fix Node 10+ while keeping
support for older versions of node

@jsf-clabot
Copy link

jsf-clabot commented Dec 5, 2017

CLA assistant check
All committers have signed the CLA.

@@ -126,7 +126,7 @@ describe("RuleTester", () => {
{ code: "var foo = bar;", errors: [{ message: "Bad error message." }] }
]
});
}, /Bad var\..*==.*Bad error message/);
}, /Bad var\.((.*==)||(strict equal)).*Bad error message/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you just need one | here.

@@ -167,7 +167,7 @@ describe("RuleTester", () => {
{ code: "var foo = bar;", errors: ["Bad error message."] }
]
});
}, /Bad var\..*==.*Bad error message/);
}, /Bad var\.((.*==)||(strictEqual)).*Bad error message/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

I agree with @j-f1's comments, although I don't think they're blockers.

Changes to the output of strictEqual are breaking the test suite
This minor adjustment to the regex will fix Node 10+ while keeping
support for older versions of node
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Dec 5, 2017

Updated with notes PTAL. Was thinking JS and not regex... || actually was causing the expression to always pass 😅

Replaced with single | and added .* to handle the whitespace. Everything appears to be working now

@not-an-aardvark not-an-aardvark added the chore This change is not user-facing label Dec 5, 2017
@not-an-aardvark not-an-aardvark merged commit 1ad3091 into eslint:master Dec 5, 2017
@not-an-aardvark
Copy link
Member

Thanks!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants