test: only refresh tmpDir for tests that need it #1954
Conversation
Note that I'd love to see some timing numbers for this, can you time a full test run with and without this change and post the results here? |
| try { | ||
| rimrafSync(exports.tmpDir); | ||
| } catch (e) { | ||
| } |
There was a problem hiding this comment.
I think this usually goes on the same line } catch (e) { }, is the linter OK with that?
There was a problem hiding this comment.
make jslint doesn't complain. There's one similar occurrence in the file (although it has a comment in the empty block so maybe that's different): https://github.com/nodejs/io.js/blob/a6b8ee19b85bbd798510191f0aee596f36b909d2/test/common.js#L159-L161
There was a problem hiding this comment.
Hmm. I'd say the comment makes it a little bit different, but i'll leave it up to you if the linter is happy either way.
Do you mean something like |
|
@Trott probably for this, I'd think |
|
yeah, just a |
|
Might as well try both |
|
Not seeing a dramatic difference running master: Branch in this PR: |
|
oh, that's disappointing! I'm supposed to send you Jenkins creds so when you get them you can run on the CI cluster and compare run times across platforms, if we get a big enough win on the ARM machines then this'd be worth it |
|
On Jenkins, armv7-ubuntu-1404 takes 13 minutes with and without this change. No measurable change. I suspect that will be the story across the board, despite my fervent hope that it shaves a few minutes of the Raspberry Pi test runs (which I guess we'll find out about in the next hour or so). Jenkins runs against:
I'm still in favor of this change. It gets rid of the code smell where child processes were detected by the presence of But I can see a counterargument that it introduces complexity compared to the previous "always refresh the tmpdir no matter what" approach. |
|
armv7-ubuntu-1404 may actually be quicker at fs operations than some of the other slaves because they have local eMMC storage, not virtualized like most of the other machines and not an SD card like the pi's. |
|
Ah, OK. I'll hold out hope a bit. I should probably be comparing test run times and not the whole time listed which will include the build time. I'm unfamiliar with Jenkins and not seeing an easy way to get a total time spent running tests. The best I've found is comparing times for individual tests in the TAP Extended Test Results. If there's an obvious better place for me to look, please clue me in. |
|
Yeah, this is a good point actually - some of the slave groups are backed by multiple machines and some of them use ccache so there's no guarantee you're going to get a similar build time! most of the standard linux slaves are backed by a single machine, all of the ARM except for the ubuntu1404 one are backed by multiples as is Windows. This might be a fruitless excercise UNLESS you push two branches that put a |
Expose `common.refreshTmpDir()` and only call it for tests that use common.tmpDir or common.PIPE. A positive side effect is the removal of a code smell where child processes were detected by the presence of `.send()`. Now each process can decide for itself if it needs to refresh tmpDir.
|
I followed your (@rvagg) suggestion to put As you can see in the table below, elapsed execution is faster (with one exception), but not always by a whole lot. The largest savings is ubuntu1404-64 shaving off 42 seconds. I have no idea what's up with ubuntu1404-32 which took 7 seconds longer. In both cases, I don't know how much of the change is ordinary variability. But I think it's probably significant that 9 out of 10 builds were faster and the one that wasn't was only a little bit slower.
|
|
thanks for the numbers, this lgtm I guess |
|
LGTM from me as well. |
Expose `common.refreshTmpDir()` and only call it for tests that use common.tmpDir or common.PIPE. A positive side effect is the removal of a code smell where child processes were detected by the presence of `.send()`. Now each process can decide for itself if it needs to refresh tmpDir. PR-URL: #1954 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
|
Merged in 7c79490 |
Expose
common.refreshTmpDir()and only call itfor tests that use common.tmpDir or common.PIPE.
A positive side effect is the removal of a code
smell where child processes were detected by the
presence of
.send(). Now each process can decidefor itself if it needs to refresh tmpDir.
/cc @rvagg (who suggested this approach in comments on #1877)