test: account for truthy signal in flaky async_hooks tests #58478
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
When the spawned child process gets closed with a signal, the exit code is not set and that is why the exit code assertion was failing. This change adjusts the test to check the signal and if it is truthy, it doesn't assert the exit code and instead logs the signal and continues the rest of the assertions. Refs: nodejs#58463 (comment) Refs: nodejs#58199 Refs: nodejs#58463 Signed-off-by: Darshan Sen <raisinten@gmail.com>
nodejs-github-bot
added
async_hooks
This comment was marked as outdated.
This comment was marked as outdated.
Flarna
approved these changes
May 27, 2025
RaisinTen
added
the
author ready
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58478 +/- ##
==========================================
+ Coverage 90.19% 90.20% +0.01%
==========================================
Files 635 635
Lines 187255 187344 +89
Branches 36775 36796 +21
==========================================
+ Hits 168896 169003 +107
+ Misses 11137 11098 -39
- Partials 7222 7243 +21 🚀 New features to boost your workflow:
|
legendecas
approved these changes
May 28, 2025
legendecas
added
the
commit-queue
lpinca
approved these changes
May 28, 2025
nodejs-github-bot
removed
the
commit-queue
|
Landed in 7ce82ad |
RaisinTen
deleted the
test-account-for-truthy-signal-in-flaky-async_hooks-tests
branch
targos
pushed a commit
that referenced
this pull request
May 31, 2025
When the spawned child process gets closed with a signal, the exit code is not set and that is why the exit code assertion was failing. This change adjusts the test to check the signal and if it is truthy, it doesn't assert the exit code and instead logs the signal and continues the rest of the assertions. Refs: #58463 (comment) Refs: #58199 Refs: #58463 Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #58478 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This was referenced Jun 3, 2025
joyeecheung
reviewed
Jun 5, 2025
| child.on('close', common.mustCall((code) => { | ||
| assert.strictEqual(code, 1); | ||
| child.on('close', common.mustCall((code, signal) => { | ||
| if (signal) { |
There was a problem hiding this comment.
I noticed this PR from #58567 and it seems to me this is more sweeping the issue under the rug, because the test should not really expect a signal to appear as it does not actively kill the child process. If the child process is killed by a signal, something is going wrong, and the changes here would be masking it. If it's just AIX machine weirdness the right workaround should probably be branching on AIX and ignore if the signal is from a known set (but SIGSEGV, etc. shouldn't be expected in any case, otherwise if someone breaks the path by introducing a memory corruption etc. the test would ignore it).
There was a problem hiding this comment.
This flaky test report #58199 of another test which is very similar to this one shows that this can happen on Linux x64 too. The failure in #58122 (comment) on 6 May, according to Jenkins, is the only place where I have seen it fail on Linux x64. I checked a couple of failures before and after that one in the sequence of Jenkins CI runs on that machine and I wasn't able to find more occurrences. The only ones that happened more frequently and got recorded in the reliability repo are the AIX failures from 20 to 28 May recently and the ancient ones from 2023.
In #58463 (comment), @Flarna suggested that this might be caused by a resource limitation.
A complete revert would reintroduce the flake whereas this could still be improved. Sent a PR: #58601.
RaisinTen
added a commit
to RaisinTen/node
that referenced
this pull request
Jun 6, 2025
This change skips asserting the exit code of the child process with the corrupted async hooks stack only on those platforms where termination via a signal has been observed. Refs: nodejs#58478 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com>
This was referenced Jun 6, 2025
nodejs-github-bot
pushed a commit
that referenced
this pull request
Jun 8, 2025
This change skips asserting the exit code of the child process with the corrupted async hooks stack only on those platforms where termination via a signal has been observed. Refs: #58478 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #58601 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95
pushed a commit
that referenced
this pull request
Jun 8, 2025
This change skips asserting the exit code of the child process with the corrupted async hooks stack only on those platforms where termination via a signal has been observed. Refs: #58478 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #58601 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95
pushed a commit
that referenced
this pull request
Jun 10, 2025
When the spawned child process gets closed with a signal, the exit code is not set and that is why the exit code assertion was failing. This change adjusts the test to check the signal and if it is truthy, it doesn't assert the exit code and instead logs the signal and continues the rest of the assertions. Refs: #58463 (comment) Refs: #58199 Refs: #58463 Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #58478 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95
pushed a commit
that referenced
this pull request
Jun 10, 2025
This change skips asserting the exit code of the child process with the corrupted async hooks stack only on those platforms where termination via a signal has been observed. Refs: #58478 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #58601 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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.
When the spawned child process gets closed with a signal, the exit code is not set and that is why the exit code assertion was failing. This change adjusts the test to check the signal and if it is truthy, it doesn't assert the exit code and instead logs the signal and continues the rest of the assertions.
Refs: #58463 (comment)
Refs: #58199
Refs: #58463