FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

LTS: backport no-unused-vars to v4.x-staging by MylesBorins · Pull Request #4688 · nodejs/node · GitHub

/ node Public

LTS: backport no-unused-vars to v4.x-staging - #4688

Merged
MylesBorins merged 11 commits into
nodejs:v4.x-stagingfrom
MylesBorins:sorry-rich
Jan 14, 2016
ConversationCommits (11)ChecksFiles changed
Merged

LTS: backport no-unused-vars to v4.x-staging#4688
MylesBorins merged 11 commits into
nodejs:v4.x-stagingfrom
MylesBorins:sorry-rich

Conversation

MylesBorins commented Jan 14, 2016

Copy link
Copy Markdown
Contributor

MylesBorins commented Jan 14, 2016

Copy link
Copy Markdown
Contributor Author

/cc @Trott @jasnell

This was referenced Jan 14, 2016
Closed
Closed

MylesBorins commented Jan 14, 2016

Copy link
Copy Markdown
Contributor Author

There was a big combination of weird things going on getting this to backport nicely. Some commits that were part of this large change landed in LTS a week ago. Other commits landed, but with conflicts, others were not tagged for LTS and were just missed.

I'd be very interested to discuss ways we can co-ordinate moving large changes like this to LTS more efficiently. As branches diverge this kind of stuff is going to get more and more difficult.

MylesBorins commented Jan 14, 2016

Copy link
Copy Markdown
Contributor Author

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

This was referenced Jan 14, 2016
Closed
Closed

Trott commented Jan 14, 2016

Copy link
Copy Markdown
Member

Ci failure for test-cluster-disconnect-race is of concern only if #4457 and #4465 have both landed in 4.x-staging. If not, then that issue should be fixed when those land.

In terms of how to prevent this in the future, the obvious things that leap to mind are:

  • More process. I suspect this is a bad idea as it will probably just make the whole thing more confusing and error-prone. For example, we could add some more tagging to note commits/PRs that depend on other commits/PRs. I don't think that will actually work, to be honest, but there's one possibility.
  • Bundling of things into mega-commits rather than individual small commits. This is an anti-pattern.
  • Bundling things into mega-PRs (with lots of small commits) rather than individual small PRs. Makes things harder to review and would probably greatly slow velocity.
  • More automation. This seems like the best idea but probably by far the hardest to implement. Straw proposal: Everything that lands on master would be tagged semver-patch, semver-minor, or semver-major. The semver-patch stuff would be merged to v4.x-staging automatically and in the same order that it landed on master. We'd need the CI in solid shape so the automated job could depend on it. There'd still be a lot of manual work when stuff didn't land cleanly (because a semver-patch depends on a previous semver-minor, for example) but that's work we do manually today anyway.

If the automation idea is appealing, maybe we can open a discussion over in the nodejs/build repo? Although I imagine getting the node-accept-pull-request job working again would be a necessary pre-requisite. (We're close, I think.)

Trott commented Jan 14, 2016

Copy link
Copy Markdown
Member

(Also: Nice work, @thealphanerd!)

mscdex added the test Issues and PRs related to the tests. label Jan 14, 2016

MylesBorins commented Jan 14, 2016

Copy link
Copy Markdown
Contributor Author

I backported #4457 and rebased against it. #4465 will likely come in next release as it only was just released in v5.4.1

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

Trott added 11 commits January 14, 2016 12:00
test: remove unused modules
dba3dc5 
Many tests use require() to import modules that subsequently never gets
used. This removes those imports and, in a few cases, removes other
unused variables from tests.

PR-URL: nodejs#4684
Reviewed-By: Myles Borins <mborins@us.ibm.com>
lib: remove unused modules
46e3fd6 
Some files in `lib` were using `require` to load modules that were
subsequently not used in the file. This removes those `require`
statements.

PR-URL: nodejs#4683
Reviewed-By: Myles Borins <mborins@us.ibm.com>
test: remove unused vars
7894f19 
Remove unused vars in tests

PR-URL: nodejs#4536
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
test: remove unused vars in ChildProcess tests
e36811b 
In addition to removing unused vars, this also fixes an instance where
booleans were set presumably to check something but then never used.
This now confirms that the events that were setting the booleans are
fired.

PR-URL: nodejs#4425
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
test: remove unused variables from net tests
8c094be 
Remove a handful of variables that are declared but never used in the
tests for the net module.

PR-URL: nodejs#4430
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
test: remove unused variables from HTTPS tests
ba1847d 
PR-URL: nodejs#4426
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
test: remove unused variables from TLS tests
7c30944 
Some of the TLS tests have variables that do not get used. This removes
those variables.

PR-URL: nodejs#4424
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
test: extend timeout in Debug mode
36d28ce 
Debug mode slows execution speed. There is work afoot to enable Debug
mode runs on the continuous integration infrastructure for the project.
Some tests are timing out, such as test-net-nodejsGH-5504.js.

This change doubles the timeout returned from `common.platformTimeout()`
when running in Debug mode. It also removes an unused variable from the
aforementioned test-net-nodejsGH-5504.js.

PR-URL: nodejs#4431
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
test: remove unused variables form http tests
eb53af2 
The http tests seem especially prone to including unused variables.
This change removes them.

PR-URL: nodejs#4422
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
test: remove unused vars from parallel tests
b3b150c 
Remove all remaining unused variables from tests in test/parallel.

PR-URL: nodejs#4511
Reviewed-By: James M Snell<jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
tools: implement no-unused-vars for eslint
ef5e9ef 
PR-URL: nodejs#4536
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins commented Jan 14, 2016

Copy link
Copy Markdown
Contributor Author

CI against current head
https://ci.nodejs.org/job/node-test-pull-request/1238/

MylesBorins commented Jan 14, 2016

Copy link
Copy Markdown
Contributor Author

CI Failures look unrelated. @jasnell are we good to land this?

jasnell commented Jan 14, 2016

Copy link
Copy Markdown
Member

LGTM

MylesBorins merged commit ef5e9ef into nodejs:v4.x-staging Jan 14, 2016

MylesBorins commented Jan 14, 2016

Copy link
Copy Markdown
Contributor Author

landed

MylesBorins deleted the sorry-rich branch January 21, 2016 22:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL