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

Jump to bottom

gh-94675: Add a regression test for rjsmin re slowdown #94685

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

gh-94675: Add a regression test for rjsmin re slowdown #94685

wants to merge 4 commits into from
+31 −1

Conversation

hroncok
Copy link
Contributor

@hroncok hroncok commented Jul 8, 2022 •
edited by bedevere-bot

@bedevere-bot bedevere-bot added awaiting review tests labels Jul 8, 2022
@hroncok hroncok mentioned this pull request Jul 8, 2022
Open
@hroncok
Copy link
Contributor Author

@hroncok hroncok commented Jul 8, 2022

I can confirm that when cherry-picked on 3.11.0b3, the test hangs. Will see what happens next. If you have some tips about how to make the tests fail when it takes longer than X seconds, other than using subprocess, please let me know.

@hroncok
fixup! pythongh-94675: Add a regression test for rjsmin re slowdown
ee602ea 
Use multiprocessing to kill the test after SHORT_TIMEOUT.
@hroncok
Copy link
Contributor Author

@hroncok hroncok commented Jul 8, 2022

If you have some tips about how to make the tests fail when it takes longer than X seconds, other than using subprocess, please let me know.

I've used multiprocessing.

hroncok
hroncok reviewed Jul 8, 2022
View changes
Lib/test/test_re.py Show resolved Hide resolved
@hroncok @arhadthedev
Fix typos
c8a850e 
Co-authored-by: Oleg Iarygin <dralife@yandex.ru>
@hroncok

This comment was marked as outdated.

Sign in to view
tiran
tiran reviewed Jul 8, 2022
View changes
Lib/test/test_re.py Outdated Show resolved Hide resolved
gpshead
gpshead approved these changes Jul 8, 2022
View changes
hauntsaninja
hauntsaninja reviewed Jul 8, 2022
View changes
Copy link
Contributor

@hauntsaninja hauntsaninja left a comment •
edited

Thanks, this looks great to me.

If there's concern over the use of multiprocessing, note that "tests that run effectively forever when broken" are a different category to me than "does not take too long" — as long as the test suite does not complete successfully, we'll detect the regression ;-)

Here's a recent example of a regression test that runs forever when regressed: #93815

@tiran
Copy link
Member

@tiran tiran commented Jul 8, 2022

My concern not about multiprocessing per se. The commit would have broken our WebAssembly buildbots. Neither WASI nor Emscripten support fork() or creation of subprocesses.

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

@tiran tiran

@arhadthedev arhadthedev

@hauntsaninja hauntsaninja

@gpshead gpshead

@serhiy-storchaka serhiy-storchaka

Assignees
No one assigned
Labels
awaiting merge expert-regex needs backport to 3.11 tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@hroncok @tiran @gpshead @arhadthedev @hauntsaninja @bedevere-bot @AlexWaygood