Refactor e2e tests to use Sandbox

[Gusarich]

Closes #510

• I have updated CHANGELOG.md
• I have documented my contribution in Tact Docs: https://github.com/tact-lang/tact-docs/pull/PR-NUMBER
• I have added tests to demonstrate the contribution is correctly implemented: this usually includes both positive and negative tests, showing the happy path(s) and featuring intentionally broken cases
• I have run all the tests locally and no test failure was reported
• I have run the linter, formatter and spellchecker
• I did not do unrelated and/or undiscussed refactorings

[Gusarich]

@anton-trunov Here's a draft of the refactor. You can look how increment.spec.ts looks now with this approach.

Another option would be to make some open method in Tracker which will wrap SandboxContract<SmartContract> into TrackedContract<...> and will parse events automatically on any send... call. But I'd like to hear your thoughts on that

[anton-trunov]

@Gusarich So do I understand correctly Tracker essentially does the following under the hood?

       tracker.parse(
            await contract.send(
                treasure.getSender(),
                { value: toNano("10") },
                { $$type: "Deploy", queryId: 0n },
            ),
        );

const deployResult = await contract.send(
            treasure.getSender(),
            { value: toNano('10') },
            { $$type: 'Deploy', queryId: 0n }
        );

expect(deployResult.transactions).toHaveTransaction({
            from: treasure.address,
            to: treasure.address,
            deploy: true,
            success: true,
        });

[Gusarich]

@Gusarich So do I understand correctly Tracker essentially does the following under the hood?

       tracker.parse(
            await contract.send(
                treasure.getSender(),
                { value: toNano("10") },
                { $$type: "Deploy", queryId: 0n },
            ),
        );

const deployResult = await contract.send(
            treasure.getSender(),
            { value: toNano('10') },
            { $$type: 'Deploy', queryId: 0n }
        );

expect(deployResult.transactions).toHaveTransaction({
            from: treasure.address,
            to: treasure.address,
            deploy: true,
            success: true,
        });

no it doesn't. tracker just parses a bunch of transactions from Sandbox into readable and clean format that was used in tact emulator

[anton-trunov]

We could do that, but I don't think having lots of unused info is really beneficial for tests.

• Checking a transaction happened is good
• Recording gas usage is not so important if it's a correctness test and not a gas usage benchmark
• Recording concrete addresses, BoCs and other noise is bad

It just creates large diffs that nobody reads. Instead of that I'd prefer to have tests like the ones in the Blueprint template for the counter contract: https://github.com/ton-org/blueprint/blob/9b1c238938047d3ca1a1b30e5706fcb759950025/src/templates/tact/counter/tests/spec.ts.template

[anton-trunov]

I don't really know what I'm supposed to get from this part of the snapshot:

[Gusarich]

I don't really know what I'm supposed to get from this part of the snapshot:

this one is an external message to treasury that initiates everything else. we can just hide that. but everything else should be parsed into readable format

[Gusarich]

We could do that, but I don't think having lots of unused info is really beneficial for tests.

• Checking a transaction happened is good
• Recording gas usage is not so important if it's a correctness test and not a gas usage benchmark
• Recording concrete addresses, BoCs and other noise is bad

It just creates large diffs that nobody reads. Instead of that I'd prefer to have tests like the ones in the Blueprint template for the counter contract: https://github.com/ton-org/blueprint/blob/9b1c238938047d3ca1a1b30e5706fcb759950025/src/templates/tact/counter/tests/spec.ts.template

so you suggest moving from snapshot-based testing to explicit checks like expect(result).toHaveTransaction(...)?

This way we won't even need Tracker and that new parseMessage thing. But it'll require much more refactoring and I don't think I'll be able to do it fast enough to fit into 1.4.2

[anton-trunov]

so you suggest moving from snapshot-based testing to explicit checks like expect(result).toHaveTransaction(...)?

I'd say we don't even need to have toHaveTransaction for most of the tests, since I wouldn't expect Sandbox to model lossy transactions (at least by default). To me some trace of contract states would represent a proof of correctness. So, we would just check the state of our contract (contracts) after each transaction to be what we expect.

[Gusarich]

so you suggest moving from snapshot-based testing to explicit checks like expect(result).toHaveTransaction(...)?

I'd say we don't even need to have toHaveTransaction for most of the tests, since I wouldn't expect Sandbox to model lossy transactions (at least by default). To me some trace of contract states would represent a proof of correctness. So, we would just check the state of our contract (contracts) after each transaction to be what we expect.

well, it depends on the case.
but anyway, to check the state of contract in a "meaningful" way we need to add get-methods to most of the e2e test contracts like in #579

[anton-trunov]

but anyway, to check the state of contract in a "meaningful" way we need to add get-methods to most of the e2e test contracts like in #579

I like the idea! We can generate it for debug builds automatically and call it something like __tactContractState

[Gusarich]

but anyway, to check the state of contract in a "meaningful" way we need to add get-methods to most of the e2e test contracts like in #579

I like the idea! We can generate it for debug builds automatically and call it something like __tactContractState

so does that mean resolving #579 first?

[anton-trunov]

so does that mean resolving #579 first?

Yeah, that's sounds like a good approach to me, but also see issue #657

[Gusarich]

@anton-trunov @novusnota I've pushed refactors of several tests. I'd like to hear your opinion and (maybe) change something in the way I do these before going further.

[Gusarich]

It seems to me now that we probably won't even need self-getters for most of the e2e tests because the things that are being tested usually have their own getters.

[anton-trunov]

@Gusarich Looks incredible!

We could probably implement some helpers to reduce boilerplate for the frequent case where you send a message and then immediately check it was successful:

For instance, this

const result = await contract.send(
            treasure.getSender(),
            { value: toNano("10") },
            { $$type: "Deploy", queryId: 0n },
        );

        expect(result.transactions).toHaveTransaction({
            from: treasure.address,
            to: contract.address,
            success: true,
            deploy: true,
        });

should probably by refactored into one such helper call deployAndCheck or something like that. And for non-deploying transactions sendAndCheck should also be useful in many cases.

[Gusarich]

@anton-trunov each contract has different available message types and it will be problematic to somehow generalize such sendAndCheck function

[novusnota]

each contract has different available message types and it will be problematic to somehow generalize such sendAndCheck function

@anton-trunov I agree with @Gusarich here, as I've tried implementing the same idea and it turned out to be a very leaky abstraction in the end

[anton-trunov]

LGTM!

[Gusarich]

@anton-trunov I think we can also safely remove stuff such as @tact-lang/ton-jest, @tact-lang/coverage and @tact-lang/ton-abi?

[anton-trunov]

We can certainly try

[novusnota]

Awesome! We indeed could try to remove @tact-lang/ton-abi, but the @tact-lang/ton-jest is used for snapshotSerializers in jest.config.json and without @tact-lang/coverage we'll need to re-make jest.setup.js and jest.teardown.js