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

Disabling flaky test_pthread_cond_signal_1_1 #13259

Conversation

abujalski
Copy link
Contributor

test_pthread_cond_signal_1_1 has very tight timing requirements, and
expects that side threads will be woken and do some action in very short
time. This causes occasional spurious fail of this test not because side
thread(s) hasn't been woken by pthread_cond_signal, but because this
threads hasn't been scheduled and hasn't get CPU on time.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

thanks!

tests/test_posixtest.py Show resolved Hide resolved
# Mark certain tests as flaky, which may sometimes fail.
# TODO invesigate these tests.
flaky = {
'test_pthread_cond_signal_1_1': 'test has tight timing requirements, may fail when test suite is run in parallel',
Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact if you just make the string here "flaky: <bug_url>" you can put the rest of the details in the bug report rather than here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, just to make things clear to me. You mean that I should file a bug as described here https://emscripten.org/docs/getting_started/bug_reports.html#bug-reports and then use URL of this newly reported bug? Or should I report this bug somewhere else?

@sbc100
Copy link
Collaborator

sbc100 commented Jan 15, 2021

Yes, file an emscripten bug called "test_pthread_cond_signal_1_1 is flaky", in which you can describe why it is flaky. Then if it ever gets fixed we can link it back to this disabled test.

In our test code if often flag disabled python tests with "@disabled(<url_to_bug>)" so that anyone auditing the disabled tests can to the bug URL to get all the details and see its already been fixed.

@sbc100
Copy link
Collaborator

sbc100 commented Jan 15, 2021

Another way of putting it: we don't ever want to disable tests without a bug filed so there can be path to re-enabling it. (Except of course for all the tests in this test suite that don't apply to emscripten.. we have no plans to fix those).

@abujalski
Copy link
Contributor Author

Ok, thanks for explanation :)

I'll file a bug and update review as suggested.

@abujalski abujalski force-pushed the disable_pthread_cond_signal_1_1_flaky_test branch from 73f8d9b to 08b74d0 Compare January 19, 2021 08:09
@abujalski
Copy link
Contributor Author

@sbc done as suggested:

test_pthread_cond_signal_1_1 has very tight timing requirements, and
expects that side threads will be woken and do some action in very short
time. This causes occasional spurious fail of this test not because side
thread(s) hasn't been woken by pthread_cond_signal, but because this
threads hasn't been scheduled and hasn't get CPU on time.

See emscripten-core#13283
@abujalski abujalski force-pushed the disable_pthread_cond_signal_1_1_flaky_test branch from 08b74d0 to 712b97d Compare January 22, 2021 14:18
@sbc100 sbc100 merged commit ac0a4b3 into emscripten-core:master Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants