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

discuss: refactor the test harness #14214

Closed
refack opened this issue Jul 13, 2017 · 7 comments
Closed

discuss: refactor the test harness #14214

refack opened this issue Jul 13, 2017 · 7 comments
Labels
discuss Issues opened for discussions and feedbacks. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. test Issues and PRs related to the tests.

Comments

@refack
Copy link
Contributor

refack commented Jul 13, 2017

Post snapshotocalipse it seems like the runtime of the test suite has grown by 100%-300%.
Till now the paradigm was self contained tests that only use common for test related utilities. Changing that might called for, or at least enabling some other, more performant approach, in parallel. Just one example of a test that run on armv7-wheezy (arbitrary but representative)

With snapshot (https://ci.nodejs.org/job/node-test-commit-arm/10793/nodes=armv7-wheezy)

ok 1620 addons-napi/test_general/testNapiStatus
  ---
  duration_ms: 0.447
  ...

Without snapshot (https://ci.nodejs.org/job/node-test-commit-arm/10832/nodes=armv7-wheezy)

ok 1622 addons-napi/test_general/testNapiStatus
  ---
  duration_ms: 2.127
  ...

Even when V8 snapshots will be re-enabled (or startup time is boosted in another way), the test suite has become quite cumbersome to run locally...

I had a few ideas:

  1. Spinning up fewer processes and running batches of tests in a vm.context. But AFAICT that has several caveats
    • I'm not sure how much of a performance boost that will give.
    • It will require triage of the tests that are compatible with running in this way. (probably require separating
      those that depend of global/process change to a separate directory
    • Parity should be keep tight between independent runs of test, and harnessed runs.
  2. Culling the test suite by using the coverage report.
    Random piece of code from the report:
    image
    show that some lines are hit 100K times, maybe we can reduce redundant tests.
  3. Try to infer the impact a change has and automatically select an optimal subset of tests to be run locally (or even as a gate condition in the CI system)

We need more ideas in order to keep the test suite tractable, otherwise it'll lose a part of it's effectiveness.

@refack refack added discuss Issues opened for discussions and feedbacks. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. test Issues and PRs related to the tests. labels Jul 13, 2017
@refack
Copy link
Contributor Author

refack commented Jul 13, 2017

/cc @nodejs/testing @nodejs/performance

@refack refack changed the title test: refactor the test harness discuss: refactor the test harness Jul 13, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Jul 13, 2017

I'm definitely -1 to removing any tests unless it can be shown that the exact same scenario is already tested.

Using the number of times a line is executed probably isn't a great metric either. For example, a complex conditional could be executed many times without being properly tested, even if both paths through the conditional are executed at least once.

@targos
Copy link
Member

targos commented Jul 13, 2017

Could we build with snapshot enabled to test on slow systems?

@mcollina
Copy link
Member

I'm -1 on changing the test harness in a rush. It's not a good idea, as I have good faith we can resolve the snapshot problem. Refactoring the test harness would cause massive issues in backporting etc.

I am +1 in enabling the snaphots on CI.

@refack
Copy link
Contributor Author

refack commented Jul 13, 2017

I'm -1 on changing the test harness in a rush.

Ack on no rush. IMHO this should be as a mid-term goal....


There are our current numbers:

image
https://ci.nodejs.org/job/node-test-commit/11091/


These are the number with snapshot:
image
https://ci.nodejs.org/job/node-test-commit/11045/

They are not amazing either.


Refactoring the test harness would cause massive issues in backporting etc.

We could set that as a goal (easy backporting).

@refack
Copy link
Contributor Author

refack commented Jul 13, 2017

I'm definitely -1 to removing any tests unless it can be shown that the exact same scenario is already tested.

Using the number of times a line is executed probably isn't a great metric either. For example, a complex conditional could be executed many times without being properly tested, even if both paths through the conditional are executed at least once.

Good point.
(I was discounting external dependencies and state)

@refack
Copy link
Contributor Author

refack commented Sep 14, 2017

Discussion done

@refack refack closed this as completed Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

4 participants