fix: verification of fuel-core node killing

[nedsalk]

Summary

This PR addresses the issue reported in #1755 (comment).

I have removed the flaky test and opted for verifying the logic via mocks. I have verified that spawning a fuel-core process group with the detached:true argument and killing the process group via process.kill(-child.pid) works and the mocks are verifying that the code is doing that.
I initially wanted to ensure that all fuel-core nodes are killed in the tests-ci.sh script by verifying that no fuel-core processes are running after pnpm test is run, but the team suggested this approach as being enough and that this external checking is not necessary.

Checklist

• I added — tests 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

[arboleya]

[..] but why does only this test suite require the await sleep(1000)?

Yes, I second this.

I am always hesitant about fixing flakiness with timeouts because the latter is usually the cause of the former.

Timeouts often seem like a quick symptomatic solution to a potentially unclear underlying cause.

[danielbate]

@nedsalk thinking out loud here, could block scoping inside the test have a better desired effect, given that using the hook seems to be flaky and that is the expected usage of the disosable.

  test('synchronous cleanup kills node before test runner exits', async () => {
    const killedNode = {
      url: '',
    };

    {
      const { cleanup, url } = await launchNode({ loggingEnabled: false });
      killedNode.url = url;
      cleanup();
    }

    await expect(fetch(killedNode.url)).rejects.toThrow('fetch failed');
  });

[nedsalk]

@FuelLabs/sdk-ts I have approached this problem from a different angle and explained my reasoning in the PR's description.

[nedsalk]

@danielbate The problem is that cleanup is synchronous whereas process killing is by it's nature asynchronous because it's done by the OS. The only 100% bulletproof way to ensure that after cleanup is called the node is killed is to make cleanup asynchronous and await some OS signal. I opted out of that because it would force us to use the async disposable pattern as explained here, which is IMO prohibitive for the DX:

using launched = await launchTestNode();
// vs
await using launched = await launchTestNode();

[maschad]

Nice job

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
fuels-ts	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 18, 2024 1:33pm

[danielbate]

Much better than await using ...

[github-actions]

Coverage Report:

Lines	Branches	Functions	Statements
79.37%(+0%)	71.65%(+0%)	77.3%(+0%)	79.5%(+0%)

Changed Files:

Coverage values did not change👌.