search

FuelLabs / fuels-ts

Fuel Network Typescript SDK
https://docs.fuel.network/docs/fuels-ts/
Apache License 2.0
44.05k stars 1.36k forks source link

Add type-level tests for `launchTestNode` #2775

Open nedsalk opened 4 months ago

nedsalk commented 4 months ago

Conceptually, you can introduce changes to a type that would still allow the compiler to compile the code successfully but that would introduce a regression as the type is not the intended one. The only way to guard against that is via type-level tests which assert that, for a given set of concrete inputs, the type inference logic returns the exact expected outputs. These type-level tests would be "green" if the compiler successfully compiles them.

We need such type-level tests so that we can catch problems like the one fixed in #2756.

maschad commented 4 months ago

I would like to see a possible instance of such a regression, the scenario in #2756 cannot be re-introduced as it stands as the tagged union was necessarily narrowed , and should someone try to revert it to a tagged union, the compiler would throw an error on the contract function calls.

Writing a test to catch problem such as the one in #2756 would be the equivalent of trying to do a contract function call on the tagged union, at which point the compiler would complain. My apprehension here is that we are doing double work, when we should just leverage the compiler.

I only see such a test offering a benefit when interacting with javascript directly.

danielbate commented 3 months ago

Do we still need this given #2579 and #2811 and our increase in test node usage across the repo? How come the test in #2756 was reverted?

nedsalk commented 3 months ago

@danielbate the problem is that we're not doing compile-time checks in our test suite, hence the usage across the repo doesn't prevent us from introducing a breaking change in the types. We either do that or we create specific tests that we'd run tsc on and where we're asserting that the types are those that we expect. I just created #2941 where type-level tests would be of clearer value. If we introduce type testing infrastructure in that issue, it could be used here as well.

danielbate commented 3 months ago

Why do we need a separate issue for #2941. Would be not do #2941 as part of this issue? Prove #2941 through type level tests?

nedsalk commented 3 months ago

@danielbate #2941 modifies the typegen outputs and is unrelated to this issue except that they both touch on type-level tests. This one is related to launchTestNode specifically, whereas #2941 is related to Interface, Contract, Predicate, Script and possibly some others. If it's a lot of clutter, we can close this issue and cover it in #2941.