test: ArbSysParserTests#163
Conversation
There was a problem hiding this comment.
Wow, lots of comprehensive tests!
Just some general comments:
-
Just for info, in some tests, you test some different edge cases for the underlying ArbSys function being called but it's not really necessary because @AnkushinDaniil's PR already tests all the edge cases. So for the parser test file, you can focus on testing its internal functions (parsing/serializing mechanisms). If the parsing/serializing worked as expected then we can usually verify only 1 successful execution by asserting the returned serialized value. Otherwise, we can assert it threw an EndOfStreamException / or other potential exception. But it doesn't hurt to have more tests if you already wrote them. I'm just trying to explain how I see the purpose of this parser test file :)
-
You can simplify tests like i mentioned in some comments where you can directly compare the serialized returned value
byte[], instead of parsing it to some other typeUInt256for example. It helps make the tests a bit shorter. -
If you could also order the tests in the same order as the ArbSysParser's functions being tested, it would really help to follow, no need to search etc :)
-
I believe using
TestCasecould really help simplify and lighten tests avoiding setting up inputs/expectations logic. -
I feel like some tests could be divided to test only 1 concept/thing, for example when you have several for loops in tests.
hudem1
left a comment
There was a problem hiding this comment.
I think the tests now look really clean and concise !!
It's really easy to follow and understand what is being tested !
And I think the file really fulfills its purpose of testing the parsing/serializing mechanism only.
Looks really nice, great job!
5750bce
into
hudem1/feat/precompile/arb-sys
* feat: First version of incomplete implementation * feat: Implementation finished * fix: Make BurnBalance public * fix: InverseAddressAliasOffset fix * refactor: Move ArbSysMetadata code to ArbSys * clean: Remove console logs * fix: Use correct types for ArbSysL2ToL1Tx and ArbSysL2ToL1Transaction structs * fix: The value passed to WithdrawEth and SendTxToL1 is the solidity msg.value * fix: DecodeEvent now does not assume non-indexed params as last * quick fix: use BigInteger instead of UInt256 as value can be negative * fix: Return SendRoot from DigestMessage instead of Hash256.Zero * refactor(send-hash): Reuse ArbosStorage.CalculateHash instead of creating a new same one * chore: Use explicit types * refactor: Rename KeccakHashWithCost to ComputeKeccakHash * refactor: Add explicit comment * chore: Add context when throwing exception * chore: Ternary if convention style * test: ArbSysParserTests (#163) * Tests & few changes * feat: Use latest version of feature/arbitrum-setup for NMC submodule with StateStack * refactor: Use existing helper * test: add `ArbSys` precompile tests (#162) * fix: Revert unintentional changes when merging test PR * fix: Forgotten merge conflict changes * fix: Code format * fix: Remove local-only changes * fix: Code format --------- Co-authored-by: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com> Co-authored-by: Daniil Ankushin <ankushin.daniil42@gmail.com>
Fixes Resolve #166
Note
The base is
hudem1/feat/precompile/arb-sysImportant
These PRs need to be merged in order for the tests to pass:
#154
#9180