-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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-async-context-frame
is flaky
#54808
Comments
I have raised this one in openjs flaky ci slack channel and @Qard has responded positively 🙏 I think he would take a look soon. |
Yeah, it appears to be a segfault (access violation) that appears to only happen on windows as far as I can determine. Will need a windows machine to investigate further. I was planning to dig in more next week on this one as I need to break out the Windows machine and update it ;-) FWIW, this appears to be a test-runner issue, |
Are you referring to the Python runner or the JS one? I'm assuming you mean Python in this case, but just wanted to verify. |
Not sure yet. I think maybe the python runner but can't be certain. I'm running an experiment in #54818 that removes both the node:test runner and the python test runner from the mix to see if we still see flakiness on these. If that looks good, I want to next try with the python runner but no |
The line throwing that is:
The |
I would be too! But just going through due diligence! It's also possible this isn't a problem with the tests at all and something wrong with the windows host running the tests! Just need to go through the steps to eliminate possible causes and narrow it down. |
I've been exploring all the results of the reported failures and they seem to fail and many different points in the run. It has a slight bias toward some particular tests but that could be a timing thing and not actually indicative of any failure of the code itself. I think I agree with @jasnell here that something seems off with running multiple python test runners in parallel. Might be worth just disabling (or reducing?) the parallelism and see how that goes? |
IIUC this has been fixed by #54823 and the conclusion is..when using |
Technically, when the test is in |
It runs each individual test separately with the runner though rather than having one run on all matches. We could probably change it to just be a single run, but I went for breaking out the tests to have some separation. But yeah, having a dozen or so parallel python runners going is going to eat a lot of memory. I was thinking about if maybe we should have a more official way to run a set of tests with extra flags like that to add to the test suite matrix. Might be an idea to define something a bit better for that, though I don't know if many other cases need it. 🤔 In any case, I think we can close this now. |
I think the problem is more like, the Python test runner concurrency should not be mixed with node:test concurrency. Say there are 8 cores, and using Python test runner to run the tests under the parallel folder, if all the tests are written using node:test which then also runs the tests concurrently, you end up with 64 processes running concurrently in total on 8 cores. Either the test using node:test should be placed in sequential folder so there are 8 processes spawned by node:test, or the test should avoid using node:test when it’s under the parallel folder and there are just 8 processes spawned by the Python test runner. |
|
Yep, I understand that. Doing it sequentially like it's done now though means there's no N magnification, it's just the outer test runner plus a singular inner test at the moment whereas before it was the runner plus N sub-processes and on top of that all the additional parallelism of the python runner running the outer test suite. If we had a better way to configure specific tests to run with multiple sets of flags this problem could be eliminated. Perhaps we could make the runner support parsing multiple sets of |
Maybe an environment variable to specify max concurrency could help, which can be used to override concurrency coming from the JS API. The Python test runner can always set it to 1 for parallel tests. (Although it may be a footgun if someone actually tries to test concurrency but forgot that it shouldn't be under parallel, though). |
It is still flaky.
|
Test
test-async-context-frame
Platform
Windows
Console output
Build links
Additional information
No response
The text was updated successfully, but these errors were encountered: