◐ Shell
reader mode source ↗
Skip to content

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

Merged
brettcannon merged 2 commits into
python:mainfrom
mariocj89:pu/threadinmock-fix
Jul 6, 2023
Merged

gh-106458: Mark testthreadingmock.py with @requires_working_threading#106366
brettcannon merged 2 commits into
python:mainfrom
mariocj89:pu/threadinmock-fix

Conversation

@mariocj89

@mariocj89 mariocj89 commented Jul 3, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@mariocj89 mariocj89 requested a review from cjw296 as a code owner July 3, 2023 14:51
@mariocj89 mariocj89 mentioned this pull request Jul 3, 2023
Merged
@tirkarthi

tirkarthi commented Jul 3, 2023

Copy link
Copy Markdown
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 Jul 3, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jul 3, 2023
Closed
@mariocj89

mariocj89 commented Jul 3, 2023

Copy link
Copy Markdown
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

bedevere-bot commented Jul 3, 2023

Copy link
Copy Markdown

🤖 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

cjw296 commented Jul 3, 2023

Copy link
Copy Markdown
Contributor

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

mariocj89 commented Jul 3, 2023

Copy link
Copy Markdown
Contributor Author

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

@mariocj89

mariocj89 commented Jul 3, 2023

Copy link
Copy Markdown
Contributor Author

Can you trigger another run of buildbots?

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

bedevere-bot commented Jul 3, 2023

Copy link
Copy Markdown

🤖 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

mariocj89 commented Jul 4, 2023

Copy link
Copy Markdown
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

brettcannon commented Jul 5, 2023

Copy link
Copy Markdown
Member

@mariocj89 unfortunately there's now a merge conflict.

@brettcannon brettcannon changed the title gh-61215: Mark testthreadingmock.py with @requires_working_threading Jul 5, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jul 5, 2023
Closed
@brettcannon

brettcannon commented Jul 5, 2023

Copy link
Copy Markdown
Member

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

@brettcannon brettcannon self-assigned this Jul 5, 2023
@cjw296 cjw296 linked an issue Jul 5, 2023 that may be closed by this pull request
test_unittest failing due to test/test_unittest/testmock/testthreadingmock assuming threading exists #106458
Closed
mariocj89 added 2 commits July 6, 2023 10:07
@mariocj89
Mark testthreadingmock.py with threading_helper.requires_working_th… …
5136773 
…reading

The test uses threading and it should declare that it uses it to prevent
failures on archs where creating threads don't work.
@mariocj89
testthreadingmock: Reduce chance of race condition …
78d9031 
Add longer delays to reduce the change of a race conditions on the tests
that validate short timeouts.
@mariocj89 mariocj89 force-pushed the pu/threadinmock-fix branch from 8b1d248 to 78d9031 Compare July 6, 2023 08:09
@mariocj89

mariocj89 commented Jul 6, 2023

Copy link
Copy Markdown
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

bedevere-bot commented Jul 6, 2023

Copy link
Copy Markdown

🤖 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
@brettcannon

brettcannon commented Jul 6, 2023

Copy link
Copy Markdown
Member

Thanks!

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

Reviewers

@cjw296 cjw296 cjw296 left review comments

@brettcannon brettcannon brettcannon approved these changes

Assignees

@brettcannon brettcannon

Labels

skip news

Projects

None yet

Milestone

No milestone

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

@mariocj89 @tirkarthi @bedevere-bot @cjw296 @brettcannon @sobolevn @AlexWaygood