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

test: extract globals & use in test-abort-backtrace #21816

Closed

Conversation

jeromecovington
Copy link
Contributor

@jeromecovington jeromecovington commented Jul 14, 2018

It was suggested that extracting globals from test/common/index.js so that it
can be required individually will pare down test dependencies could ultimately
improve test performance.

See discussion on this issue, especially the linked comment:
#20128 (comment)

This diff demonstrates first steps towards not needing to require
test/common/index.js in all tests.

If enough/all the dependent tests were refactored, benchmarks could be shown.
Although, it seems sensible to break those forthcoming refactors into separate,
reasonably small PRs.

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

Sorry, something went wrong.

First steps towards not needing to require test/common/index.js
everywhere.

Refs: nodejs#20128 (comment)
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 14, 2018
@jeromecovington
Copy link
Contributor Author

jeromecovington commented Jul 14, 2018

@Trott @nodejs/testing

@Trott
Copy link
Member

Trott commented Jul 15, 2018

@jeromecovington Welcome and thanks for the pull request. This seems to do the opposite of what I suggested in the comment you link to. I was suggesting getting everything except the global leak detection out of common/index.js because it's the only thing that is needed in (nearly) every test. In contrast, this removes the global leak detection--that is, it removes the one thing I suggested not removing.

If there's a good reason for going this way rather than the way I suggested, I can be persuaded. It's not obvious to me what the benefit is.

@jeromecovington
Copy link
Contributor Author

@Trott thanks for the welcome! Yes, I see your point. This diff extracts global leak detection to its own file and then requires only global leak detection in test-abort-backtrace.js, showing that by requiring only global leak detection in a dependent file, tests can still pass. It seems to me that the filename for the global leak detection is immaterial.

I'd need to spend more time pulling apart test dependencies before taking a heavier touch with common/index.js. In my mind I pictured refactoring all tests that only have a dependency on global leak detection to use this extracted file.

How many tests only depend on global leaks detection? Is there benefit in pursuing this?

@Trott
Copy link
Member

Trott commented Jul 15, 2018

@jeromecovington Nearly every test requires global leak detection, which is why I propose leaving it in common/index.js and moving all other functionality to individual files.

@jeromecovington
Copy link
Contributor Author

@Trott awesome thanks for the feedback. I’ll get a little deeper into the tests then.

Just to clarify, I didn’t intend this diff to be the end of it. I had thought with global leak detection extracted, that any test that only needed it could be refactored to use only it. I figured at some point, the place I am requiring global in index would be removed.

What I didn’t account for, are tests that might rely on global and another piece or two (but not all) from what is currently in index.

Effectively this diff only points to one path of optimization: either you only need global in a test, in which case you proceed as I have demonstrated, or you need global plus some other stuff, in which case you still use index.

Let me see how far I can get coming at this from the other side.

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.

None yet

3 participants