Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: launchNode.cleanup not killing node in last test of test group #2718

Merged
merged 39 commits into from
Jul 11, 2024

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented Jul 7, 2024

Summary

Launching a fuel-core node via launchNode and killing it via the cleanup function wouldn't kill the last node in a test group because the test runner would exit before the node was killed and the node wouldn't get killed. The other nodes would get killed because there was enough time for the killing to happen before the last test finishes. Launching the nodes in a detached state and killing them via process.kill(-child.pid) did the trick.

Besides this, multiple cleanup cases have been ensured to work as a result of the discussion in #2718 (comment).

Breaking Changes

The killNode and KillNodeParams functionality has been internalized and the method and interface have been deleted so they're no longer exported. It's marked as a breaking change for pedantic reasons and there shouldn't really be any affected users given that they kill nodes via cleanup which is unchanged, so no migration notes are necessary.

Checklist

  • I addedtests to prove my changes
  • I updated — all the necessary docs
  • I reviewed — the entire PR myself, using the GitHub UI
  • I described — all breaking changes and the Migration Guide

@nedsalk nedsalk added the bug Issue is a bug label Jul 7, 2024
@nedsalk nedsalk added this to the 0.x mainnet milestone Jul 7, 2024
@nedsalk nedsalk self-assigned this Jul 7, 2024
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Nice job on this, ran it locally and pgrep fuel-core comes back clean 😄 (As opposed to master which will report a pid)

Left some comments

packages/account/src/test-utils/launchNode.ts Show resolved Hide resolved
packages/account/src/test-utils/launchNode.ts Outdated Show resolved Hide resolved
@nedsalk nedsalk requested a review from maschad July 8, 2024 07:41
@nedsalk nedsalk dismissed stale reviews from Torres-ssf and petertonysmith94 via 1babf70 July 9, 2024 14:31
@nedsalk nedsalk force-pushed the ns/fix/launch-node-cleanup branch from fe648f9 to dc647cb Compare July 10, 2024 15:50
@nedsalk nedsalk force-pushed the ns/fix/launch-node-cleanup branch from dc647cb to e7abaee Compare July 10, 2024 15:55
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Nice job!

Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.41%(+0%) 71.65%(-0.02%) 77.3%(+0.27%) 79.54%(+0.06%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/test-utils/launchNode.ts 98.26%
(-1.74%)
84.74%
(-1.22%)
100%
(+21.43%)
98.34%
(+3.43%)

@nedsalk nedsalk merged commit 98dbfbb into master Jul 11, 2024
19 checks passed
@nedsalk nedsalk deleted the ns/fix/launch-node-cleanup branch July 11, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node started via launchTestNode isn't closed in tests
5 participants