[backport v10.x] test: fix pty test hangs on aix #28826
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
test
This comment has been minimized.
This comment has been minimized.
|
Noticed more than one 10.x backport PRs with perma-fail AIX tests. Hoping backporting this fixes it.... |
|
/ping @bnoordhuis (original author) @BethGriggs (person I'm hoping will get this on v10.x-staging quickly if it ends up fixing the issues) |
|
Looks like AIX tests all passed but then some build/JUnit stuff blew up. Trying again... |
|
AIX tests passed again but then there was an Java/Jenkins failure. 😞 |
|
Trying again, but forcing a different AIX host to see if it makes a difference.... |
|
OK, so this will have to also backport 1245aca to pass, it would seem. |
|
OK, added a second backport commit per above. Let's try CI again.... |
|
OK, so tests are passing now and the build problems from the stringbytes tests are no longer a thing...but now there are leftover |
|
Let's see if adding 3c9c89d from #28469 fixes it... |
|
Hooray! That fixed it! (Will need to re-run because of a spurious failure on another platform, but AIX is green.) @nodejs/releasers Is it OK to pile up three commits from three different PRs like this? I hope so... |
|
Can you perhaps apply them in order? Or at least check against master because none of the pseudo-tty tests are skipped on AIX on master. |
These tests seem to trigger failures in the entire CI job (not just the test) on AIX. Skip them to see if that helps alleviate spurious failures in node-test-commit-aix (and the upstream PR and commit test jobs). See: - nodejs/build#1820 (comment) - nodejs/build#1847 (comment) PR-URL: nodejs#28469 Backport-PR-URL: nodejs#28826 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Add SKIP status for more tests in stringbytes-external-exceed-max that are failing on AIX. PR-URL: nodejs#28516 Backport-PR-URL: nodejs#28826 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Some pty tests persistently hung on the AIX CI buildbots. Fix that by adding a helper script that properly sets up the pty before spawning the script under test. On investigation I discovered that the test runner hung when it tried to close the slave pty's file descriptor, probably due to a bug in AIX's pty implementation. I could reproduce it with a short C program. The test runner also leaked file descriptors to the child process. I couldn't convince python's `subprocess.Popen()` to do what I wanted it to do so I opted to move the logic to a helper script that can do fork/setsid/etc. without having to worry about stomping on state in tools/test.py. In the process I also uncovered some bugs in the pty module of the python distro that ships with macOS 10.14, leading me to reimplement a sizable chunk of the functionality of that module. And last but not least, of course there are differences between ptys on different platforms and the helper script has to paper over that. Of course. Really, this commit took me longer to put together than I care to admit. Caveat emptor: this commit takes the hacky ^D feeding to the slave out of tools/test.py and puts it in the *.in input files. You can also feed other control characters to tests, like ^C or ^Z, simply by inserting them into the corresponding input file. I think that's nice. Fixes: nodejs/build#1820 Fixes: nodejs#28489 PR-URL: nodejs#28600 Backport-PR-URL: nodejs#28826 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Rebased and applied in the order in which they landed. |
This comment has been minimized.
This comment has been minimized.
richardlau
approved these changes
Jul 24, 2019
BethGriggs
approved these changes
Jul 24, 2019
|
+1 to fast track |
3 tasks
|
/ping @nodejs/backporters. AIX tests on v10.x-staging will be red until this lands. |
ZYSzys
approved these changes
Jul 25, 2019
|
+1 to fast track. |
BethGriggs
pushed a commit
that referenced
this pull request
Jul 25, 2019
These tests seem to trigger failures in the entire CI job (not just the test) on AIX. Skip them to see if that helps alleviate spurious failures in node-test-commit-aix (and the upstream PR and commit test jobs). See: - nodejs/build#1820 (comment) - nodejs/build#1847 (comment) PR-URL: #28469 Backport-PR-URL: #28826 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Rich Trott <[email protected]>
BethGriggs
pushed a commit
that referenced
this pull request
Jul 25, 2019
Add SKIP status for more tests in stringbytes-external-exceed-max that are failing on AIX. PR-URL: #28516 Backport-PR-URL: #28826 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
BethGriggs
pushed a commit
that referenced
this pull request
Jul 25, 2019
Some pty tests persistently hung on the AIX CI buildbots. Fix that by adding a helper script that properly sets up the pty before spawning the script under test. On investigation I discovered that the test runner hung when it tried to close the slave pty's file descriptor, probably due to a bug in AIX's pty implementation. I could reproduce it with a short C program. The test runner also leaked file descriptors to the child process. I couldn't convince python's `subprocess.Popen()` to do what I wanted it to do so I opted to move the logic to a helper script that can do fork/setsid/etc. without having to worry about stomping on state in tools/test.py. In the process I also uncovered some bugs in the pty module of the python distro that ships with macOS 10.14, leading me to reimplement a sizable chunk of the functionality of that module. And last but not least, of course there are differences between ptys on different platforms and the helper script has to paper over that. Of course. Really, this commit took me longer to put together than I care to admit. Caveat emptor: this commit takes the hacky ^D feeding to the slave out of tools/test.py and puts it in the *.in input files. You can also feed other control characters to tests, like ^C or ^Z, simply by inserting them into the corresponding input file. I think that's nice. Fixes: nodejs/build#1820 Fixes: #28489 PR-URL: #28600 Backport-PR-URL: #28826 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
|
Landed on |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Original PRs: #28600 #28516 #28469
Some pty tests persistently hung on the AIX CI buildbots. Fix that by
adding a helper script that properly sets up the pty before spawning
the script under test.
On investigation I discovered that the test runner hung when it tried
to close the slave pty's file descriptor, probably due to a bug in
AIX's pty implementation. I could reproduce it with a short C program.
The test runner also leaked file descriptors to the child process.
I couldn't convince python's
subprocess.Popen()to do what I wantedit to do so I opted to move the logic to a helper script that can do
fork/setsid/etc. without having to worry about stomping on state in
tools/test.py.
In the process I also uncovered some bugs in the pty module of the
python distro that ships with macOS 10.14, leading me to reimplement
a sizable chunk of the functionality of that module.
And last but not least, of course there are differences between ptys
on different platforms and the helper script has to paper over that.
Of course.
Really, this commit took me longer to put together than I care to admit.
Caveat emptor: this commit takes the hacky ^D feeding to the slave out
of tools/test.py and puts it in the *.in input files. You can also feed
other control characters to tests, like ^C or ^Z, simply by inserting
them into the corresponding input file. I think that's nice.
Fixes: nodejs/build#1820
Fixes: #28489
PR-URL: #28600
Reviewed-By: Richard Lau [email protected]
Reviewed-By: Sam Roberts [email protected]
Reviewed-By: Anna Henningsen [email protected]
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes