Skip to content

Conversation

Tony133
Copy link
Contributor

@Tony133 Tony133 commented Jun 8, 2025

Checklist

Proposal:

  • migrated graceful-shutdown.test.js from tap to node:test 🔥

@Tony133 Tony133 force-pushed the test/migrated-graceful-shutdown-test branch from 2a31fae to 2954630 Compare June 8, 2025 20:39
@Tony133 Tony133 force-pushed the test/migrated-graceful-shutdown-test branch from 2954630 to 61c443c Compare June 8, 2025 20:43
const sigintHandler = () => {
try {
sinon.assert.called(spy)
t.pass('fastify.close() was called on SIGINT')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should check this with a t.plan.
Especially because t.pass is not part of the node test runner, so this test should fail

Copy link
Contributor

Choose a reason for hiding this comment

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

The Plan hasn't been inserted.
The assertion could be unchecked and the test would still pass.

Copy link
Contributor Author

@Tony133 Tony133 Aug 19, 2025

Choose a reason for hiding this comment

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

The test fails if I put the method "plan"
screenshot-error-plan

will probably fail again the CI

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of being async, you should end the test once the assertion has been checked
Contrary to tap, the test runner doesn't wait for the plan to complete

Tony133 and others added 2 commits August 20, 2025 09:46
Co-authored-by: Manuel Spigolon <[email protected]>
Signed-off-by: Antonio Tripodi <[email protected]>
@Tony133
Copy link
Contributor Author

Tony133 commented Aug 20, 2025

Most likely, the problem is related to how the SIGINT signal is handled in the CI environment. For now, I have set it to skip the test with Windows, Linux, and macOS.

I don't know if this is the right thing to do 😄, see commit here: 0e5cef3

Tony133 and others added 3 commits August 20, 2025 14:45
Co-authored-by: Manuel Spigolon <[email protected]>
Signed-off-by: Antonio Tripodi <[email protected]>
Co-authored-by: Manuel Spigolon <[email protected]>
Signed-off-by: Antonio Tripodi <[email protected]>
Co-authored-by: Manuel Spigolon <[email protected]>
Signed-off-by: Antonio Tripodi <[email protected]>
process.exit()
})

test('should call fastify.close() on SIGINT', { skip: isWindows || isMacOS || isLinux }, (t) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

if this will not run on windows, macos and linux..it means essentially that will never run
We should keep the check as it was (so just skip windows) and check what's happening

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants