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

Add Eslint rule prefer-assert-methods #8622

Closed
wants to merge 5 commits into from
Closed

Add Eslint rule prefer-assert-methods #8622

wants to merge 5 commits into from

Conversation

danyshaanan
Copy link
Contributor

@danyshaanan danyshaanan commented Sep 17, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines

(Commits will be squashed and properly named after some more review)

Description of change

@Trott

Add ESLint rule for catching uses of assert() with binary operators (===, !===, ==, !=) which could be replaces with assert's methods - strictEqual/notStrictEqual/equal/notEqual.

Here is some of the output for:

./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules test/

image

for a total of 86 errors.

If the rule will be deemed good, I'd be happy to fix these violations. (fixed).

WDTY?

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Sep 17, 2016
@danyshaanan danyshaanan changed the title Eslint Add Eslint rule prefer-assert-methods Sep 17, 2016
Copy link
Contributor

@yorkie yorkie left a comment

Choose a reason for hiding this comment

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

LGTM except an ESLint error...

'!==': 'notStrictEqual',
'==': 'equal',
'!=': 'notEqual'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Semi-colon is missing i guess :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops!! :O
Fixed.

@danyshaanan
Copy link
Contributor Author

Fixed all violations in dada443, so now make -j8 test passes.

Copy link
Contributor

@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

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is good

@Trott
Copy link
Member

Trott commented Sep 17, 2016

@danyshaanan
Copy link
Contributor Author

danyshaanan commented Sep 18, 2016

This failed in CI on the 'test/arm-fanned' check, on which most builds failed lately, after a lengthy period of time. This also applies to this PR I've opened. Am I correct in guessing that this is an issue with the 'test/arm-fanned' execution and not the PRs? How can it be determined with better certainty?

@Trott
Copy link
Member

Trott commented Sep 18, 2016

Am I correct in guessing that this is an issue with the 'test/arm-fanned' execution and not the PRs?

Yes.

How can it be determined with better certainty?

Unfortunately, I think in this case it requires familiarity with the issue affecting those devices. But generally, it's a matter of looking at the console results in the Jenkins interface.

I suspect the issue will be resolved-ish soon. The resolution will turn the builds yellow, not green, but close enough for jazz. It's a case of those devices running the tests with --flaky-tests=run' when it should be running them with--flaky-tests=dontcare`. I suspect someone on @nodejs/build will get to the bottom of it soon.

@jbergstroem
Copy link
Member

LGTM

@jbergstroem
Copy link
Member

The python install was for some reason broken on that machine; had to manually fix it (reinstall didn't work)

@JacksonTian
Copy link
Contributor

LGTM

1 similar comment
@yorkie
Copy link
Contributor

yorkie commented Sep 19, 2016

LGTM

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

jasnell pushed a commit that referenced this pull request Sep 20, 2016
PR-URL: #8622
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: Jackson Tian <shvyo1987@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
@jasnell
Copy link
Member

jasnell commented Sep 20, 2016

Landed in d469321! Thank you!

@jasnell jasnell closed this Sep 20, 2016
@MylesBorins
Copy link
Contributor

@danyshaanan glad to see you put this together!

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
PR-URL: #8622
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: Jackson Tian <shvyo1987@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants