chore: remove skip from the waitForResult test

[mvares]

• Closes: Debug waitForResult integration test #2773

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

[mvares]

The timeout defined here doesn't seem sufficient to cover the test duration. Do we need to increase the time?

fuels-ts/packages/fuel-gauge/src/transaction-response.test.ts

Lines 203 to 204 in 9a39997

	cleanup();
	}, 18500);

[petertonysmith94]

The timeout defined here doesn't seem sufficient to cover the test duration. Do we need to increase the time?

fuels-ts/packages/fuel-gauge/src/transaction-response.test.ts

Lines 203 to 204 in 9a39997

	cleanup();
	}, 18500);

Did you experience any issues with this test timing out locally?

[mvares]

Did you experience any issues with this test timing out locally?

Nops.

[petertonysmith94]

Did you experience any issues with this test timing out locally?

Nops.

I believe we can leave it for now - if it becomes an issue down the line then we can increment the timeout.

[arboleya]

CI is failing for all node versions.

[mvares]

CI is failing for all node versions.

Yes. As mentioned above, the CI environment doesn't work exactly like the local environment. I believe we still need to increase it.

[arboleya]

On the topic of fixing flakiness with timeouts:

• fix: verification of fuel-core node killing #2759 (comment)

[mvares]

On the topic of fixing flakiness with timeouts:

• fix: verification of fuel-core node killing #2759 (comment)

What's your advice? Should we wait for #2759 to be merged?

Assumptions made

[petertonysmith94]

On the topic of fixing flakiness with timeouts:

• fix: verification of fuel-core node killing #2759 (comment)

What's your advice? Should we wait for #2759 to be merged?

I would say that we're not fixing flakyness here - as the POA is 17s, using the default Vitest timeout (5s) is no sufficient for the given test.

An increment to the timeout might be worthwhile after looking closer at the CI failures.

Maybe a timeout of 22s might be optimal, given that the POA is 17s and default timeout is 5s?

[mvares]

An increment to the timeout might be worthwhile after looking closer at the CI failures.

Maybe a timeout of 22s might be optimal, given that the POA is 17s and default timeout is 5s?

Well, I think that 22s is reasonable. We can try it.

[arboleya]

I don't have the proper context and cannot dive into this right now to determine what should be done.

Besides flakiness/timeouts, another concern is when tests require so much time to run.

These intricacies here may be the reason why we were skipping the test.

[mvares]

The duration for keep-alive messages is already defined. What we can do is increase the time (test) or enforce a different duration. However, I think this is out of the test's scope because we wouldn't have a test reflecting reality.

[maschad]

@mvares thanks for taking this on, the impetus for the issue #2773 was to debug why it is that the test was failing and perhaps refactor it so that we can properly test waitForResult always waits on the transaction to be successfully completed. This test still fails when using our launchTestNode utility, even with an appropriate block production time, so I think we'd need to revisit how we are handling keep alive messages and potentially refactor getSubscriptionStreamFromFetch, I'm not sure yet, but I don't think increasing the timeout is a viable option.

This task is a bit complex and involved, I think someone from the team should take it on. My apologies about that.

[mvares]

Right. Should I close this PR?

[maschad]

Right. Should I close this PR?

Yes, thanks for the effort.