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

gh-106458: Mark testthreadingmock.py with @requires_working_threading #106366

Merged
merged 2 commits into from Jul 6, 2023

Conversation

mariocj89
Copy link
Contributor

@mariocj89 mariocj89 commented Jul 3, 2023

@tirkarthi
Copy link
Member

Please add "test-with-buildbots" label once complete so that you can run the PR on all buildbots without merging that will help in testing this.

@AlexWaygood AlexWaygood changed the title Mark testthreadingmock.py with requires_working_threading gh-61215: Mark testthreadingmock.py with @requires_working_threading Jul 3, 2023
@mariocj89
Copy link
Contributor Author

@tirkarthi can you add the label please?

@AlexWaygood AlexWaygood added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 3, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @AlexWaygood for commit cddd99d 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 3, 2023
@cjw296
Copy link
Contributor

cjw296 commented Jul 3, 2023

Looks like this test is still sad on WASM:

======================================================================
FAIL: test_wait_until_any_call_keywords (test.test_unittest.testmock.testthreadingmock.TestThreadingMock.test_wait_until_any_call_keywords)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/buildbot/bcannon-wasm/pull_request.bcannon-wasm.emscripten-node-pthreads/build/Lib/test/test_unittest/testmock/testthreadingmock.py", line 192, in test_wait_until_any_call_keywords
    self.assertNotIn(call(a=1), something.method_1.mock_calls)
AssertionError: call(a=1) unexpectedly found in [call(a=1)]
----------------------------------------------------------------------

Given the diff in this PR, I wonder why it's still being run?

@mariocj89
Copy link
Contributor Author

That tests a race condition. Seems the two lines after that take more than 100ms, let me fix it

@mariocj89
Copy link
Contributor Author

Can you trigger another run of buildbots?

@AlexWaygood AlexWaygood added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 3, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @AlexWaygood for commit f3d8389 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 3, 2023
@mariocj89
Copy link
Contributor Author

This should be good to land @cjw296, the failing build bots are in venv, extensions or grammar, which is unrelated from the change we did.

@brettcannon
Copy link
Member

@mariocj89 unfortunately there's now a merge conflict.

@brettcannon brettcannon changed the title gh-61215: Mark testthreadingmock.py with @requires_working_threading gh-106458: Mark testthreadingmock.py with @requires_working_threading Jul 5, 2023
@brettcannon
Copy link
Member

While we wait on the merge conflict to be fixed, I opened #106458 to track this specific test failure.

@@ -133,9 +137,7 @@ def test_wait_failed_with_timeout_override(self):

with patch(f"{__name__}.Something", waitable_mock):
something = Something()
self.run_async(something.method_1, delay=0.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

@mariocj89 - this change and the ones below confuse me, are they intentional or just a result of a bad attempt of mine to resolve the conflicts?

Copy link
Contributor Author

@mariocj89 mariocj89 Jul 6, 2023

Choose a reason for hiding this comment

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

This is indeed part of the change. Trying to reduce the chance of a race condition (as that's what this test is doing though)

…reading

The test uses threading and it should declare that it uses it to prevent
failures on archs where creating threads don't work.
Add longer delays to reduce the change of a race conditions on the tests
that validate short timeouts.
@mariocj89
Copy link
Contributor Author

I've just rebased all change son top of main, should be good to go

@sobolevn sobolevn added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 6, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @sobolevn for commit 78d9031 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 6, 2023
@brettcannon brettcannon merged commit 56353b1 into python:main Jul 6, 2023
78 of 97 checks passed
@brettcannon
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_unittest failing due to test/test_unittest/testmock/testthreadingmock assuming threading exists
7 participants