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

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

Merged
merged 11 commits into from
Jan 14, 2016

Conversation

@MylesBorins
Copy link
Contributor Author

/cc @Trott @jasnell

This was referenced Jan 14, 2016
@MylesBorins
Copy link
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
Copy link
Contributor Author

@Trott
Copy link
Member

Trott commented Jan 14, 2016

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
Copy link
Member

Trott commented Jan 14, 2016

(Also: Nice work, @thealphanerd!)

@mscdex mscdex added the test Issues and PRs related to the tests. label Jan 14, 2016
@MylesBorins
Copy link
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
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>
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>
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>
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>
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>
PR-URL: nodejs#4426
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
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>
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>
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>
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>
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
Copy link
Contributor Author

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

@MylesBorins
Copy link
Contributor Author

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

@jasnell
Copy link
Member

jasnell commented Jan 14, 2016

LGTM

@MylesBorins MylesBorins merged commit ef5e9ef into nodejs:v4.x-staging Jan 14, 2016
@MylesBorins
Copy link
Contributor Author

landed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants