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

Use strict equality comparisons in RuleTester #9417

Closed
not-an-aardvark opened this Issue Oct 12, 2017 · 4 comments

Comments

Projects
2 participants
@not-an-aardvark
Member

not-an-aardvark commented Oct 12, 2017

As of ESLint v4.8, RuleTester performs loose equality comparisons here for values like error line numbers. For example, providing the string "3" or the array ["3"] as an expected line number will cause the test to pass, even when the actual line number is the number 3. I think this isn't ideal, for the same reason that loose equality is a bad idea in general (it can conceal potential errors in code).

I think we should update RuleTester to use strict equality checks. This would be a breaking change since it could cause some new test failures, but I suspect very few people are relying on this behavior.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Oct 12, 2017

Adding this to the TSC agenda because I don't expect it to be contentious, and there is a TSC meeting later today anyway.

TSC Summary: This proposes using strict equality checks in RuleTester rather than loose equality checks. It would be a breaking change.
TSC Question: Should we accept this proposal?

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Oct 13, 2017

In today's meeting, the TSC decided to accept this as a breaking change for 5.0.

@not-an-aardvark not-an-aardvark added this to Accepted in v5.0.0 Oct 13, 2017

@Orlandster

This comment has been minimized.

Contributor

Orlandster commented Oct 16, 2017

@not-an-aardvark I could do that. Do I understand it right, you want to replace all of the assert.equal with assert.strictEqual ?

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Oct 16, 2017

@Orlandster1998 Yes, and we also want to replace this assert.deepEqual with assert.deepStrictEqual.

That said, keep in mind that this is scheduled as a breaking change for the v5.0.0 release, which probably isn't going to happen for a few months. So if you do make a PR now, we won't end up merging it for a few months, and it's possible you would need to fix merge conflicts when we do end up merging it. Because of this, I had been planning to make a PR closer to when we do the v5.0.0 release. (That said, if you want to make a PR now, I certainly won't stop you.)

not-an-aardvark added a commit that referenced this issue Feb 23, 2018

@not-an-aardvark not-an-aardvark moved this from Accepted, ready to implement to Ready to merge in v5.0.0 Feb 23, 2018

not-an-aardvark added a commit that referenced this issue Feb 23, 2018

v5.0.0 automation moved this from Ready to merge to Done Mar 22, 2018

@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.