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: fix tick-processor tests and run them in CI #23100

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

Those tests were doing significantly to much work and almost all failed for me before with timeouts after 2 minutes. The test was super heavy and there was a lot of IO involved. I reduced the runtime of each of these tests to about 1-3 seconds (depending on finding a match right away or if they have to try again) and moved them to sequential, so we would actually know when they fail.

I removed one of the tests as it seems that the V8 internals do not produce "UNKNOWN" frequently anymore and it was already very rare when the test was written. In the end these tests are mainly redundant to each other anyway, as it should be enough to have a single test to check for the failure and another one to check for a specific output. Checking for specific output with specific code should not be required but I thought I keep the two redundant ones anyway.

This is still WIP as I want to see if they actually still fail on a lot of OS or if we are able to run them on most systems by now.

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 26, 2018
@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Sep 26, 2018
@richardlau
Copy link
Member

Note that the existing tests do not catch #22825 (comment).

@BridgeAR BridgeAR closed this Oct 2, 2018
@BridgeAR BridgeAR reopened this Oct 2, 2018
@BridgeAR BridgeAR force-pushed the fix-prof-tests branch 2 times, most recently from 0233517 to f9fdd79 Compare October 3, 2018 08:24
@BridgeAR BridgeAR changed the title WIP: test: fix tick-processor tests and run them in CI test: fix tick-processor tests and run them in CI Oct 3, 2018
@BridgeAR BridgeAR removed the wip Issues and PRs that are still a work in progress. label Oct 3, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Oct 3, 2018

Copy link
Member

@mcollina mcollina 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 Dec 5, 2018

Needs a rebase.

@antsmartian
Copy link
Contributor

ping @BridgeAR 🔉

@Trott
Copy link
Member

Trott commented Dec 23, 2018

These tests were removed from the main suite because:

  • The tick-processor tests are based on probabilities. They therefore have false negatives from time to time. (That is, they are flaky.)

  • They also sometimes leave extra processes running. This will cause test runs to fail in CI.

If we could make sure these are resolved before moving to the CI suite, that would be 👍.

@BridgeAR
Copy link
Member Author

@Trott

These tests were removed from the main suite because: [...] That is, they are flaky. [...] They also sometimes leave extra processes running. [...] If we could make sure these are resolved before moving to the CI suite, that would be +1.

That is pretty much exactly what I tried to achieve here. It is still difficult to guarantee that the tests are not flaky though. So far they should not leave any extra processes running. The flakiness is significantly reduced but not yet completely fixed. I am working on that.

This reduces the runtime significantly and fixes the tests to run on
the CI. It is significantly more robust by accepting some fallbacks.
Therefore they are moved out of pummel to sequential.
@BridgeAR
Copy link
Member Author

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR closed this Jan 20, 2020
@BridgeAR BridgeAR deleted the fix-prof-tests branch January 20, 2020 11:51
@BridgeAR BridgeAR restored the fix-prof-tests branch January 20, 2020 11:58
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.

6 participants