Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

hoodmane
Copy link
Contributor

@hoodmanehoodmane commentedNov 22, 2024•
edited by bedevere-appbot
Loading

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

Copy link
Contributor

@mdboommdboom 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.

hoodmane and erlend-aasland reacted with thumbs up emoji
@bedevere-app
Copy link

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 phraseI 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
ContributorAuthor

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

freakboy3742 and erlend-aasland reacted with laugh emoji

@hoodmanehoodmane changed the titlegh-127146 Emscripten: Fix segfaults in test suitegh-127146 Emscripten: Skip segfaults in test suiteNov 23, 2024
After this, Emscripten makes it all the way through the test suite when I run itlocally.
@hoodmanehoodmaneforce-pushed thefix-emscripten-test-segfaults branch fromc11b73b toc6425eaCompareNovember 23, 2024 17:43
@hoodmane
Copy link
ContributorAuthor

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

@hoodmane
Copy link
ContributorAuthor

Nevermind that is not true.

@erlend-aaslanderlend-aasland changed the titlegh-127146 Emscripten: Skip segfaults in test suitegh-127146: Emscripten: Skip segfaults in test suiteNov 26, 2024
Copy link
Contributor

@freakboy3742freakboy3742 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 torequires_subprocess (e.g.,requires_full_stack?) so that the skip message is consistent.

erlend-aasland reacted with thumbs up emoji
@hoodmane
Copy link
ContributorAuthor

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

Thanks for making the requested changes!

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

Copy link
Contributor

@freakboy3742freakboy3742 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

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 phraseI 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
ContributorAuthor

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

@bedevere-app
Copy link

Thanks for making the requested changes!

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

@erlend-aaslanderlend-aasland removed their request for reviewDecember 2, 2024 21:43
Copy link
Contributor

@freakboy3742freakboy3742 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.

Copy link
Member

@Eclips4Eclips4 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
ContributorAuthor

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

@freakboy3742freakboy3742 merged commit43634fc intopython:mainDec 5, 2024
39 checks passed
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull requestJan 8, 2025
)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 freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@freakboy3742freakboy3742freakboy3742 approved these changes

@mdboommdboommdboom approved these changes

@Eclips4Eclips4Eclips4 approved these changes

@barneygalebarneygaleAwaiting requested review from barneygalebarneygale is a code owner

@iritkatrieliritkatrielAwaiting requested review from iritkatrieliritkatriel is a code owner

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

@corona10corona10Awaiting requested review from corona10corona10 is a code owner

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

Successfully merging this pull request may close these issues.

4 participants
@hoodmane@freakboy3742@mdboom@Eclips4
[8]ページ先頭
©2009-2025 Movatter.jp