test: fix flaky doctool and test - #29979
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
|
Relevant Windows CI is green. Taking this out of Draft mode. Collaborators please 👍 here to fast-track to unbreak CI. |
Sorry, something went wrong.
Sorry, something went wrong.
|
Stress test won't work for this because doctool isn't built in that job, but in addition to the green Windows run in the regular CI, here's four more Windows CI runs with this code to confirm it fixes the doctool test issue there: ✅ = Windows job was fully green https://ci.nodejs.org/job/node-test-binary-windows-2/3531/ ✅ |
Sorry, something went wrong.
|
And to show it failing on master, here are five from master: ❌= doctool test failed on win2008r2 https://ci.nodejs.org/job/node-test-binary-windows-2/3532/ ❌ |
Sorry, something went wrong.
Given the failure is so prominent on master are we sure it's not a real recent regression? The test has only failed in the two most recent node-daily-master runs (out of the twelve we have history for): The change LGTM btw but I just want to make sure we're not papering over something that has recently changed to suddenly make this test fail consistently. |
Sorry, something went wrong.
Pretty sure it's not a regression. I bisected (by running tests on CI) and the supposed problematic change was 5f80df8 which seems rather unlikely to me given that there's no http in this test. |
Sorry, something went wrong.
|
(One more 👍 to approve fast-tracking over at #29979 (comment) please?) |
Sorry, something went wrong.
The likely scenarios to me seem to be that either that change (or some other nearby change) caused a timing change on win2008r2 such that it triggered this issue a lot more, or that something changed on the Windows host/configuration itself to make this problem occur far more often. (But yes, I'm speculating.) |
Sorry, something went wrong.
Not in the tests themselves but the doctool itself does a https request (hence #29918). |
Sorry, something went wrong.
|
I'm going to bed because it's far too late here, but if this gets another fast track approval and someone wants to land it, please do! Bonus points for going to the CI jobs associated with #29976, #29969, and #15735 and using "Rebuild" to start them over again with this change. |
Sorry, something went wrong.
Old PRs are still passing when run on CI, so that suggests that the second suggestion by me isn't correct. The first one seems likely: A code change caused a timing change on win2008r2 such that it triggered this issue a lot more. |
Sorry, something went wrong.
There was a problem hiding this comment.
These could now be fsPromises.writeFile(...), although I can do that in a follow up if you prefer.
Sorry, something went wrong.
Doctool tests have been failing a lot in CI on Win2008 R2. It appears async functions and callback-based functions are being used in combination such that the callback-based function cannot guarantee that it will invoke its callback. Convert the callback-based functions to async functions so we have one paradigm and reliable results. PR-URL: #29979 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Doctool tests have been failing a lot in CI on Win2008 R2. It appears async functions and callback-based functions are being used in combination such that the callback-based function cannot guarantee that it will invoke its callback. Convert the callback-based functions to async functions so we have one paradigm and reliable results. PR-URL: #29979 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Doctool tests have been failing a lot in CI on Win2008 R2. It appears async functions and callback-based functions are being used in combination such that the callback-based function cannot guarantee that it will invoke its callback. Convert the callback-based functions to async functions so we have one paradigm and reliable results. Backport-PR-URL: nodejs#32642 PR-URL: nodejs#29979 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Doctool tests have been failing a lot in CI on Win2008 R2. It appears async functions and callback-based functions are being used in combination such that the callback-based function cannot guarantee that it will invoke its callback. Convert the callback-based functions to async functions so we have one paradigm and reliable results. Backport-PR-URL: #32642 PR-URL: #29979 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Doctool tests have been failing a lot in CI on Win2008 R2. It appears
async functions and callback-based functions are being used in
combination such that the callback-based function cannot guarantee that
it will invoke its callback. Convert the callback-based functions to
async functions so we have one paradigm and reliable results.
Checklist