[v8.x] Backport aix test fixes to v8.x#29599
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or 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 
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#29599 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#29599 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#1820Fixes: nodejs#28489 PR-URL: nodejs#28600 Backport-PR-URL: nodejs#29599 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
andrewhughes101force-pushed the backport-aix-to-v8.x branch from ee87649 to f8b2ffdCompare
Collaborator
nodejs-github-bot commented Sep 18, 2019
BethGriggs approved these changes Sep 18, 2019
sam-github approved these changes Sep 18, 2019
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
richardlau approved these changes Sep 18, 2019
Assumption that if memory can be malloc()ed it can be used is not true on AIX. Later access of the allocated pages can trigger SIGKILL if there are insufficient VM pages. Use psdanger() to better estimate available memory. Fixes: nodejs/build#1849 More info: - https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/generalprogramming/sys_mem_alloc.html - https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/p_bostechref/psdanger.html Related to: - nodejs/build#1820 (comment) - nodejs#28469 - nodejs#28516 PR-URL: nodejs#28857 Backport-PR-URL: nodejs#29599 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Rich Trott <[email protected]>
One skipped test remains, it creates very large Buffer objects, triggering the AIX OOM to kill node and its parent processes. See: nodejs/build#1849 (comment) PR-URL: nodejs#29054 Backport-PR-URL: nodejs#29599 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Collaborator
nodejs-github-bot commented Sep 19, 2019
Member
BethGriggs commented Sep 19, 2019
Landed on |
BethGriggs pushed a commit that referenced this pull request Sep 19, 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: #29599 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 Sep 19, 2019
Add SKIP status for more tests in stringbytes-external-exceed-max that are failing on AIX. PR-URL: #28516 Backport-PR-URL: #29599 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 Sep 19, 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#1820Fixes: #28489 PR-URL: #28600 Backport-PR-URL: #29599 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 19, 2019
Assumption that if memory can be malloc()ed it can be used is not true on AIX. Later access of the allocated pages can trigger SIGKILL if there are insufficient VM pages. Use psdanger() to better estimate available memory. Fixes: nodejs/build#1849 More info: - https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/generalprogramming/sys_mem_alloc.html - https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/p_bostechref/psdanger.html Related to: - nodejs/build#1820 (comment) - #28469 - #28516 PR-URL: #28857 Backport-PR-URL: #29599 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 19, 2019
One skipped test remains, it creates very large Buffer objects, triggering the AIX OOM to kill node and its parent processes. See: nodejs/build#1849 (comment) PR-URL: #29054 Backport-PR-URL: #29599 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
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.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesrefs: #28600
refs: #28516
refs: #28469