NotificationsYou must be signed in to change notification settings
Fork32k
Star67.3k

gh-132162: tests for py_universalnewlinefgets#132164

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

Jump to bottom

Merged

vstinner merged 1 commit intopython:mainfromalex-semenyuk:tests_py_universalnewlinefgets

Apr 24, 2025

Merged

gh-132162: tests for py_universalnewlinefgets#132164

vstinner merged 1 commit intopython:mainfromalex-semenyuk:tests_py_universalnewlinefgets

Apr 24, 2025

Conversation

Copy link

Contributor

alex-semenyuk commentedApr 6, 2025•
edited by bedevere-appbot
Loading

Cover by tests Py_Universalnewlinefgets as mentioned atTODO

Issue:Tests for Py_Universalnewlinefgets #132162

bedevere-appbot added the awaiting review label

Apr 6, 2025

bedevere-appbot mentioned this pull request

Apr 6, 2025

Tests for Py_Universalnewlinefgets#132162

Closed

alex-semenyuk force-pushed thetests_py_universalnewlinefgets branch 2 times, most recently from1bfd558 toc8cd3acCompare

April 6, 2025 16:47

Copy link

ContributorAuthor

alex-semenyuk commentedApr 12, 2025

There is just
`0:01:23 load avg: 9.59 [101/486/1] test_capi worker non-zero exit code (Exit code 3221226505)

Copy link

ContributorAuthor

alex-semenyuk commentedApr 12, 2025

^^@picnixz

Copy link

Member

picnixz commentedApr 12, 2025

Do you need help? or are you asking for a review? for help on Windows-related stuff I'm not the one to ask in general (I'm not a Windows expert). My guess is that the issue comes from the newlines on Windows (\r\n) vs Unix (\n).

alex-semenyuk marked this pull request as draft

April 12, 2025 08:52

bedevere-appbot removed the awaiting review label

Apr 12, 2025

alex-semenyuk force-pushed thetests_py_universalnewlinefgets branch 2 times, most recently from2c71597 to03f2e95Compare

April 12, 2025 09:15

Copy link

ContributorAuthor

alex-semenyuk commentedApr 12, 2025•
edited
Loading

Do you need help? or are you asking for a review? for help on Windows-related stuff I'm not the one to ask in general (I'm not a Windows expert). My guess is that the issue comes from the newlines on Windows (\r\n) vs Unix (\n).

From this log it's not quite clear where the issue is exact, just mentioned testcapi

picnixz added the skip news label

Apr 12, 2025

picnixz requested changes

Apr 12, 2025

View reviewed changes

Copy link

Member

picnixz left a comment

Choose a reason for hiding this comment

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

There are multiple leaks and Windows needs_fdopen notfdopen IIRC.

Modules/_testcapi/file.c OutdatedShow resolvedHide resolved

Modules/_testcapi/file.cShow resolvedHide resolved

Modules/_testcapi/file.c Outdated

		}
		FILE *fp = fdopen(fd, "rb");
		if (fp == NULL) {
		PyErr_SetFromErrno(PyExc_OSError);

Copy link

Member

picnixzApr 12, 2025

Choose a reason for hiding this comment

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

this should return NULL

Modules/_testcapi/file.cShow resolvedHide resolved

bedevere-appbot added the awaiting changes label

Apr 12, 2025

Copy link

bedevere-appbot commentedApr 12, 2025

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.

And if you don't make the requested changes, you will be poked with soft cushions!