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.

Sign up for GitHub

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

Jump to bottom

test,tools: enable linting for undefined vars in tests #6255

Closed
wants to merge 1 commit into from
Closed

test,tools: enable linting for undefined vars in tests #6255

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 18, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

test tools

Description of change

The test directory had linting for undefined variables disabled. It is
enabled everywhere else in the code base. Let's disable the fule for
individual lines in the handful of tests that use undefined variables.

@Trott Trott added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Apr 18, 2016
@Trott
Copy link
Member Author

Trott commented Apr 18, 2016

@nodejs/testing

@santigimeno
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/2295/

@santigimeno
Copy link
Member

All green except from #6133

LGTM

cjihrig
cjihrig reviewed Apr 18, 2016
View reviewed changes
test/message/nexttick_throw.js
@@ -5,7 +5,9 @@ process.nextTick(function() {
process.nextTick(function() {
process.nextTick(function() {
process.nextTick(function() {
/* eslint-disable no-undef */
Copy link
Contributor

Choose a reason for hiding this comment

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

Does our version of ESLint support eslint-disable-next-line? It would make these types of changes only require one line.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's in there since v2.2.0, so yes, it would be supported.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 18, 2016

LGTM with one suggested change if it's supported.

@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

LGTM

1 similar comment
@silverwind
Copy link
Contributor

LGTM

@Trott
test,tools: enable linting for undefined vars
87306ff 
The test directory had linting for undefined variables disabled. It is
enabled everywhere else in the code base. Let's disable the fule for
individual lines in the handful of tests that use undefined variables.
@Trott
Copy link
Member Author

Trott commented Apr 20, 2016

Took @cjihrig's suggestion, then rebased and force pushed.

New CI: https://ci.nodejs.org/job/node-test-pull-request/2334/

@Trott
Copy link
Member Author

Trott commented Apr 20, 2016

CI is good except for known flaky tests.

jasnell pushed a commit that referenced this pull request Apr 20, 2016
@Trott @jasnell
test,tools: enable linting for undefined vars
4faaed6 
The test directory had linting for undefined variables disabled. It is
enabled everywhere else in the code base. Let's disable the fule for
individual lines in the handful of tests that use undefined variables.

PR-URL: #6255
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

Landed in 4faaed6

@jasnell jasnell closed this Apr 20, 2016
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@Trott
test,tools: enable linting for undefined vars
48ecc0b 
The test directory had linting for undefined variables disabled. It is
enabled everywhere else in the code base. Let's disable the fule for
individual lines in the handful of tests that use undefined variables.

PR-URL: #6255
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
This was referenced Apr 21, 2016
Closed
Closed
Closed
@MylesBorins
Copy link
Contributor

@Trott lts?

MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
@Trott @MylesBorins
test,tools: enable linting for undefined vars
fb2a978 
The test directory had linting for undefined variables disabled. It is
enabled everywhere else in the code base. Let's disable the fule for
individual lines in the handful of tests that use undefined variables.

PR-URL: #6255
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@Trott
Copy link
Member Author

Trott commented Apr 21, 2016

@thealphanerd If it lands cleanly, yes.

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@Trott @joelostrowski
test,tools: enable linting for undefined vars
767d908 
The test directory had linting for undefined variables disabled. It is
enabled everywhere else in the code base. Let's disable the fule for
individual lines in the handful of tests that use undefined variables.

PR-URL: nodejs#6255
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@Trott @jasnell
test,tools: enable linting for undefined vars
422faba 
The test directory had linting for undefined variables disabled. It is
enabled everywhere else in the code base. Let's disable the fule for
individual lines in the handful of tests that use undefined variables.

PR-URL: #6255
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request May 17, 2016
@Trott
test,tools: enable linting for undefined vars
5570e2e 
The test directory had linting for undefined variables disabled. It is
enabled everywhere else in the code base. Let's disable the fule for
individual lines in the handful of tests that use undefined variables.

PR-URL: #6255
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
@Trott
test,tools: enable linting for undefined vars
c76f214 
The test directory had linting for undefined variables disabled. It is
enabled everywhere else in the code base. Let's disable the fule for
individual lines in the handful of tests that use undefined variables.

PR-URL: #6255
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@MylesBorins MylesBorins mentioned this pull request May 18, 2016
Merged
@Trott Trott deleted the undefyo branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Trott @santigimeno @cjihrig @jasnell @silverwind @MylesBorins