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.

Sign up for GitHub

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

Jump to bottom

test: run faster and cleanup after run #8848

Closed
wants to merge 1 commit into from
Closed

test: run faster and cleanup after run #8848

wants to merge 1 commit into from

Conversation

geek
Copy link
Member

@geek geek commented Sep 29, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Fixes #8725

tick tests now run faster and cleanup spawned processes. Previously, when executed continuously the tick tests would leave spawned processes after the test completed.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 29, 2016
@mscdex mscdex added the process Issues and PRs related to the process subsystem. label Sep 29, 2016
@Trott
Copy link
Member

Trott commented Sep 30, 2016

/cc @indutny @matthewloring

@Trott Trott mentioned this pull request Sep 30, 2016
Closed
2 tasks
@MylesBorins
Copy link
Contributor

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

imyller
imyller approved these changes Sep 30, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

matthewloring
matthewloring approved these changes Sep 30, 2016
View reviewed changes
Copy link

@matthewloring matthewloring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Trott
Copy link
Member

Trott commented Sep 30, 2016

CI again now that a few issues in CI have been cleaned up:https://ci.nodejs.org/job/node-test-commit/5392/

indutny
indutny suggested changes Sep 30, 2016
View reviewed changes
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with cleaning it up on exit, but I don't see huge benefit from using asynchronous functions here. Is there much point in it?

@Trott
Copy link
Member

Trott commented Sep 30, 2016

(CI is yellow. First time in a long time. Green can't be too far behind, can it? CAN IT?!?!)

@Trott
Copy link
Member

Trott commented Oct 3, 2016 •
edited
Loading

/ping @geek Any response to this from @indutny?:

I don't see huge benefit from using asynchronous functions here. Is there much point in it?

These test failures have been a long-standing significant problem and I'd really love to get that question resolved so we can land this. Let me know if you'd like me to do anything.

EDIT: Oh, these are test improvements but they don't solve the regular test failures. My mistake.

@geek
Copy link
Member Author

geek commented Oct 3, 2016

@indutny if you think moving back to sync is better, I can try that... I do think this is easy enough to reason about as it is written. Do you want this to be sync?

@jbergstroem
Copy link
Member

New CI here: https://ci.nodejs.org/job/node-test-commit/5440/

If this PR intends to fix #8725 we should put a log in the fire. That bug has been coloring our test environment red for a while..

@geek
Copy link
Member Author

geek commented Oct 4, 2016

@indutny reverted to sync style. The tests are still faster and more reliable!

@indutny
Copy link
Member

indutny commented Oct 4, 2016

Looks much better now, thank you! Few questions:

  • What particular change yielded the major speed up?
  • Are the other changes necessary to maintain that speed up? Or if put in other words, do they bring any running time improvement?

@geek
Copy link
Member Author

geek commented Oct 4, 2016

@indutny first, cleaning up the child processes... I noticed that they would often remain after the tests all ran. After this cleanup, the time between retrying to read the ticks and find the expected value was reduced, which is really where the speedup came from.

I will go through the remaining tests and see if I can find similar issues.

@Trott Trott mentioned this pull request Oct 4, 2016
Closed
2 tasks
@Trott
Copy link
Member

Trott commented Oct 4, 2016

Curious if this helps flakiness on the Raspberry Pi devices.

Stress test on current master: https://ci.nodejs.org/job/node-stress-single-test/976/nodes=pi2-raspbian-wheezy/console

Stress test on this PR: https://ci.nodejs.org/job/node-stress-single-test/977/nodes=pi2-raspbian-wheezy/console

@Trott
Copy link
Member

Trott commented Oct 4, 2016

For that matter, let's try the Pi 3 too:

master: https://ci.nodejs.org/job/node-stress-single-test/978/nodes=pi3-raspbian-jessie/console

this PR: https://ci.nodejs.org/job/node-stress-single-test/979/nodes=pi3-raspbian-jessie/console

@geek
Copy link
Member Author

geek commented Oct 5, 2016

@indutny do you approve the PR?

@geek
Copy link
Member Author

geek commented Oct 6, 2016

All tests are green, I reran linux-ppc and it passed: https://ci.nodejs.org/job/node-test-commit-plinux/4619/

@indutny after you approve the PR it will be ready for merge.

jasnell
jasnell approved these changes Oct 6, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@indutny
Copy link
Member

indutny commented Oct 6, 2016

Approved! LGTM. Thank you

@jasnell jasnell self-assigned this Oct 6, 2016
jasnell pushed a commit that referenced this pull request Oct 6, 2016
@geek @jasnell
test: run faster and cleanup after run
b88612d 
PR-URL: #8848
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Matthew Loring <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 6, 2016

Landed in b88612d. Thank you!

@jasnell jasnell closed this Oct 6, 2016
@MylesBorins
Copy link
Contributor

lts?

@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

Possibly. @indutny what do you think about pulling this back to v4?

@indutny
Copy link
Member

indutny commented Oct 8, 2016

I'm fine with backporting it

jasnell pushed a commit that referenced this pull request Oct 10, 2016
@geek @jasnell
test: run faster and cleanup after run
74c772c 
PR-URL: #8848
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Matthew Loring <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@geek @Fishrock123
test: run faster and cleanup after run
9b0733f 
PR-URL: #8848
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Matthew Loring <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
@MylesBorins
Copy link
Contributor

bah... the test doesn't exist in v4.x... changing to do not land

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imyller imyller imyller approved these changes

@matthewloring matthewloring matthewloring approved these changes

@jasnell jasnell jasnell approved these changes

@indutny indutny indutny approved these changes

Assignees

@jasnell jasnell

Labels
process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@geek @Trott @MylesBorins @jbergstroem @indutny @jasnell @matthewloring @imyller @mscdex @nodejs-github-bot