tests: reduce runtime by BridgeAR · Pull Request #20688 · nodejs/node

BridgeAR · 2018-05-12T16:40:46Z

This refactors some tests to reduce the runtime of those.

I looked at the first five tests from #20128 but the other two did not seem to be
possible to improve (CPU bound + a necessary timeout of 1 second).

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

BridgeAR · 2018-05-12T16:41:22Z

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

Trott · 2018-05-12T17:40:18Z

Rather than move the module use to a later line, thus making this test violate our test formatting guidelines (and possibly make it inevitable that someone will just move it back some day soon), it would be better IMO to not call this file with spawnSync at all but instead to move the recursive async call stuff to a fixture. Is there a reason not to do that instead?

+1 on this... the require('../common') should be at the top.

Loading common is a immense overhead and when looking into that file there are only two reasons for it to always load currently:

On exit it checks if there are "leaked globals". I do not understand the reasoning for these checks. What is the point in checking for these? If anyone adds such a thing it should probably come up in a code review.

If the NODE_TEST_WITH_ASYNC_HOOKS environment variable is set, it will check for some things on exit. The reasoning for this was probably a rough smoke test. I personally believe we should not use it like this and instead just try to add a solid test base that would cover these things already. Do we know if this has ever uncovered some bugs?

Besides that common only exports functions for convenience.

So what I actually want to propose is to remove all common requires if they are not necessary and I would also like to split common into a couple small files that are specific to the individual use case. This will hopefully also improve the test runtime in general.

The check for leaked globals should remain. It may be missed in code review and people should be notified when they run make test rather than having it come up during code review.

I'd be fine if everything else were moved to individual modules under common but that's a big change.

But why would we manipulate globals in a test if there is no reason to do so?

Having a general check against leaked globals is absolutely fine but we could achieve that by just requiring all modules in a test and then checking for the known globals.

@BridgeAR You make a reasonable case for removing the requirement of always loading common. I'm not sure I'm convinced--for example, if the current global leak detection approach does not work sufficiently well, might not the better approach be to fix it rather than to not require it? I'm asking rhetorically, as I can see arguments both ways on that one.

If no one else objects to it, I won't either.

That all aside: if we want to run some code in all our tests files all the time (no matter what code), should we not just rewrite the way how we call our tests and make sure the tests automatically load that code as well?

That had been suggested in the past and, IIRC, got a big -1 from people (I think @bnoordhuis was one of them) because it introduces more magic. In other words, tests will pass when called with test.py or whatever but fail if run directly with ./node. For me personally, I'm not sure that's really a problem. It might even be an improvement from our current situation if it changes us from "some tests fail if you don't use test.py" to "all tests fail if you don't use test.py". But I don't feel strongly about that.

@nodejs/testing

Actually, thinking more about it, I'm not 100% sure the case that it doesn't work correctly is convincing. So what if it doesn't check non-enumerables? That just means we fail to detect it if someone adds a non-enumerable. First, that can be fixed. But second, in most cases, I'd imagine people accidentally leak enumerables. And those get detected. Or am I missing something?

Yeah, it's only taken me four minutes, but I think I've returned to thinking that the global leak detection is valuable and should be in all tests.

For me it still seems weird to have a halfway broken test that runs with each test file. Yes, it does detect enumerable entries but that should also be possible in almost all cases by just having a single test file that loads all modules. Of course the global could be added somewhere deep down but that is even more unlikely... I also do not know a compelling reason for not manipulating globals in tests. We normally do not do that but if it is done, it would only impact that specific test. For me removing common is more about code hygiene than about runtime.

It feels like this is a legacy test that was implemented because 8 years ago the test suite was not that good and people pushed code directly to the repo instead of having code reviews. All that changed and as such this test became somewhat obsolete out of my perspective.

I do not want to spend much more energy into this but I really think we should start questioning some old decisions way more often.

…I really think we should start questioning some old decisions way more often.

💯 to that. I think that's what we're doing here right now. We're just not (yet, at least) agreeing on what the correct answer to that questioning is. And you raise excellent points. I'm not convinced it outweighs the benefits, but it would be interesting to know what others think. AFACT, it's just you, me, @jasnell, and @TimothyGu that have weighed on in the specific issue. Might be interesting to see what others think. This is a significant enough issue that I can see reconstituting the Testing WG if there are one or two other similar issues like this that need to be hashed out.

Trott · 2018-05-12T17:41:37Z

Same here. Can we move the child process code to a fixture?

I think moving these things to a fixture is the wrong thing to do. We have a lot of fixtures and there is no benefit this in a fixture as far as I see it. It is much easier to handle this test if everything is together. The fixtures folder is full with things of which probably no one has an idea about what is really in there.

mscdex · 2018-05-12T19:30:33Z

I think the commit message prefix should be just test:?

TimothyGu

-1 on changing the location of require('../').

BridgeAR · 2018-05-13T20:38:53Z

@TimothyGu would you be so kind and give some reasoning for it? I just fail to understand what the point of always loading it is as I pointed out above.

Trott · 2018-05-13T21:34:14Z

@TimothyGu would you be so kind and give some reasoning for it? I just fail to understand what the point of always loading it is as I pointed out above.

@BridgeAR The purpose of this change seems to be to shave a second or two off the test run. That seems like an insignificant gain compared to the cost of introducing inconsistency in test formatting and practices, especially given that there is another approach that should be as effective (which is to put the code that doesn't need common into fixtures). I agree that fixtures is a mess, but in my opinion, the answer isn't "don't use fixtures". The answer is "organize the fixtures".

BridgeAR · 2018-05-14T01:36:36Z

@Trott for me it the current discussion is more about the general purpose of always loading common and not about shaving of a second of the test run. This would not only make the tests faster but also make the code cleaner. It just does not seem necessary to do that because the global check never really worked properly. See #20688 (comment) for more about that.

About organizing the fixtures: at least for me the main aspect is that I do not have to check another file if I want to know what the test actually does. I strongly believe it is best to keep that part in the same file.

This refactors some tests to reduce the runtime of those. Refs: nodejs#20128

BridgeAR · 2018-05-14T12:04:47Z

Rebased to address the tests comment. I also moved the common part back to the top of the files.

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

TimothyGu · 2018-05-16T13:57:08Z

-const ret = spawnSync(process.execPath, [__filename, 'async']);
+const ret = spawnSync(
+  process.execPath,
+  ['--stack_size=50', __filename, 'async']


Aren’t we never supposed to change the stack size?

Since this lowers the size that V8 assumes to have, it should be fine. This is also the only main reason for this test to run ~3x faster. I personally think it is fine to use here because it would still work even if the flag would be a no-op. @hashseed @bmeurer are you fine with me using this in this case?

TimothyGu · 2018-05-16T13:57:50Z

+);
 assert.strictEqual(ret.status, 0);
-assert.ok(!/async.*hook/i.test(ret.stderr.toString('utf8', 0, 1024)));
+const stderr = ret.stderr.toString('utf8', 0, 2048);


Why the increase in end?

I checked the actual output and it was cut off relatively early without printing a lot of information. By increasing that limit it actually makes sure we really test for the right values.

nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label May 12, 2018

Trott reviewed May 12, 2018

View reviewed changes

TimothyGu suggested changes May 13, 2018

View reviewed changes

test: reduce runtime

5cc7f93

This refactors some tests to reduce the runtime of those. Refs: nodejs#20128

BridgeAR force-pushed the improve-tests branch from f3763fd to 5cc7f93 Compare May 14, 2018 12:04

TimothyGu reviewed May 16, 2018

View reviewed changes

Conversation

BridgeAR commented May 12, 2018

Checklist

Uh oh!

BridgeAR commented May 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented May 12, 2018

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented May 13, 2018

Uh oh!

Trott commented May 13, 2018

Uh oh!

BridgeAR commented May 14, 2018

Uh oh!

BridgeAR commented May 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott May 13, 2018 •

edited

Loading

Trott May 14, 2018 •

edited

Loading

Trott May 14, 2018 •

edited

Loading