Skip to content

gh-127146: Emscripten: Skip segfaults in test suite #127151

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.

Sign up for GitHub

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
Merged
merged 6 commits into from
Dec 5, 2024
Merged

gh-127146: Emscripten: Skip segfaults in test suite #127151

merged 6 commits into from
Dec 5, 2024

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Nov 22, 2024 •
edited by bedevere-app bot
Loading

After this, Emscripten makes it all the way through the test suite when I run it locally.

@bedevere-app bedevere-app bot mentioned this pull request Nov 22, 2024
Open
@hoodmane hoodmane force-pushed the fix-emscripten-test-segfaults branch 2 times, most recently from a281d38 to c11b73b Compare November 22, 2024 14:37
@rhettinger rhettinger removed their request for review November 22, 2024 20:16
mdboom
mdboom requested changes Nov 23, 2024
View reviewed changes
Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

Sorry if this sounds overly pedantic, but isn't this "skipping tests that segfault" rather than "fixing tests that segfault"? It's a completely reasonably path to skip these tests and then come back later to fix them (if they matter), but I would suggest changing the PR title / commit message.

@bedevere-app
Copy link

bedevere-app bot commented Nov 23, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@hoodmane
Copy link
Contributor Author

As a mathematician I have no right to call anyone pedantic under practically any circumstances.

@hoodmane hoodmane changed the title gh-127146 Emscripten: Fix segfaults in test suite gh-127146 Emscripten: Skip segfaults in test suite Nov 23, 2024
@hoodmane
pythongh-127146 Emscripten: Skip segfaults in test suite
c6425ea 
After this, Emscripten makes it all the way through the test suite when I run it
locally.
@hoodmane hoodmane force-pushed the fix-emscripten-test-segfaults branch from c11b73b to c6425ea Compare November 23, 2024 17:43
@hoodmane
Copy link
Contributor Author

Seems like I can remove a lot of these xfails if I set -sSTACK_SIZE=10mb instead of 5mb.

@hoodmane
Copy link
Contributor Author

Nevermind that is not true.

@erlend-aasland erlend-aasland changed the title gh-127146 Emscripten: Skip segfaults in test suite gh-127146: Emscripten: Skip segfaults in test suite Nov 26, 2024
freakboy3742
freakboy3742 requested changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The set of changes all looks fine to me.

However, if this is going to be a common mode of failure for tests (which this PR seems to indicate it will be), I'd rather see it pulled into a utility decorator, analogous to requires_subprocess (e.g., requires_full_stack?) so that the skip message is consistent.

@hoodmane
Copy link
Contributor Author

Okay @freakboy3742 I made a common method for the stack-overflow skips. I also updated the description according to @mdboom's complaint.

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Nov 30, 2024

Thanks for making the requested changes!

@freakboy3742, @mdboom: please review the changes made to this pull request.

freakboy3742
freakboy3742 requested changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

One possible cleanup that I should have found on my first pass. There's also a bunch of CI failures that I'm fairly sure are due to one misapplied test decorator.

@bedevere-app
Copy link

bedevere-app bot commented Dec 2, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@hoodmane
Copy link
Contributor Author

hoodmane commented Dec 2, 2024

Okay, I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Dec 2, 2024

Thanks for making the requested changes!

@mdboom, @freakboy3742: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from freakboy3742 December 2, 2024 12:04
@erlend-aasland erlend-aasland removed their request for review December 2, 2024 21:43
freakboy3742
freakboy3742 approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for those fixes!

@freakboy3742
Copy link
Contributor

@mdboom Do the changes @hoodmane has made address your concerns? I'm happy to merge in the current state.

Eclips4
Eclips4 approved these changes Dec 3, 2024
View reviewed changes
Copy link
Member

@Eclips4 Eclips4 left a comment

Choose a reason for hiding this comment

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

test_ast changes looks good to me.

@hoodmane
Copy link
Contributor Author

hoodmane commented Dec 4, 2024

If @mdboom doesn't come back to approve this, maybe you can merge it @freakboy3742? I think I addressed his comment...

@freakboy3742 freakboy3742 merged commit 43634fc into python:main Dec 5, 2024
39 checks passed
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
@hoodmane @srinivasreddy
pythongh-127146: Emscripten: Skip segfaults in test suite (python#127151
027ac70 
)

Added skips for tests known to cause problems when running on Emscripten. 
These mostly relate to the limited stack depth on Emscripten.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freakboy3742 freakboy3742 freakboy3742 approved these changes

@mdboom mdboom mdboom approved these changes

@Eclips4 Eclips4 Eclips4 approved these changes

@barneygale barneygale Awaiting requested review from barneygale barneygale is a code owner

@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hoodmane @freakboy3742 @mdboom @Eclips4