Closed Gusarich closed 1 month ago
@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
@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 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
We could do that, but I don't think having lots of unused info is really beneficial for tests.
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
I don't really know what I'm supposed to get from this part of the snapshot:
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
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
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.
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
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 https://github.com/tact-lang/tact/issues/579
I like the idea! We can generate it for debug
builds automatically and call it something like __tactContractState
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?
so does that mean resolving https://github.com/tact-lang/tact/issues/579 first?
Yeah, that's sounds like a good approach to me, but also see issue #657
@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.
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.
@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.
@anton-trunov each contract has different available message types and it will be problematic to somehow generalize such sendAndCheck
function
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 I think we can also safely remove stuff such as @tact-lang/ton-jest
, @tact-lang/coverage
and @tact-lang/ton-abi
?
We can certainly try
Closes #510
I have documented my contribution in Tact Docs: https://github.com/tact-lang/tact-docs/pull/PR-NUMBER