-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
libcxx: Unskip some condvar tests #4721
libcxx: Unskip some condvar tests #4721
Conversation
…sults.txt of libcxx
These 3 tests still failed since the elapsed time was less than the timeout.
Perhaps we should skip them again. |
You want me to modify the test cases to skip these ones? maybe I can try doing that. |
Yeah, there are definitely remaining sporadic failures which will need to be re-skipped, thanks @frederick-vs-ja. From looking at the logs, I agree that those 3 are the ones that need to be re-skipped. @Arup-Chauhan Please note that you can rerun just the affected test directories, which is way faster than a full test pass. (I do appreciate that you ran all of the tests at least once - that increases our confidence that the tests work on more than just maintainer and CI machines! 😻) On my local machine, the first time I ran these directories they passed, but the second time I got one of those tests to fail. Here's how I ran the affected directories and then inspected the logs: Click to expand how I ran the tests:
|
@microsoft-github-policy-service agree |
Perfect, thanks! I expect to create a merge batch this week (after performing a Patch Tuesday toolset update tomorrow). |
Thanks! @StephanTLavavej , looking forward to more contributions! |
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Sure, I feel for now we will be waiting for the LLVM or other condvar/condvarany issues raised here that are directly related to this. |
Two more condvarany tests failed sporadically in the MSVC-internal repo ( |
It was a U+200B ZERO WIDTH SPACE.
@StephanTLavavej is there any work to be done here? As in, are there any modifications to be done in the expectd_test_results.txt file, in order to get this PR to be merged? Or will we wait to work on Issue #4723 first? |
Good question - the answer is that no action is needed, I've already updated the PR and will land it soon. This is part of the boring work that I take care of so contributors can have fun 😹 |
Just to mention @StephanTLavavej, thank you for making things easier for us! Cheers to my first PR merge as well 😄 |
Thanks for improving this test coverage, and congratulations on your second microsoft/STL commit! 🐈 🚀 😸 |
Addressing the removal of lines mentioning condvar or condvarany in tests/libcxx/expected_results.txt. These tests were previously skipped due to reliance on timing assumptions, which have been resolved upstream.
No reference issue was raised for this PR to fix
Context: The skipped tests mentioned in the text file were dependent on timing assumptions. The upstream issue has been addressed and fixed in this LLVM commit.
Changes: Every line mentioning condvar or condvarany in tests/libcxx/expected_results.txt has been removed.
Rationale: The LLVM repository is used as a submodule. Once the problem is fixed in the submodule, we can unskip these tests, ensuring they run as intended.
Testing
Execution: Ran all the tests across all test suites to confirm the removal of condvar and condvarany entries.
Results: Tests were executed as expected, ensuring no adverse effects from the removal.