Skip to content
This repository has been archived by the owner on Mar 25, 2024. It is now read-only.

Tests for L2EthToken.sol Contract #88

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from

Conversation

neotheprogramist
Copy link

Copy link
Contributor

@AntonD3 AntonD3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall tests look good, I left some small comments about the code itself. And can you add some more test cases:

  • For simple methods: balanceOf, decimals, symbol, and name. Maybe just one simple test case per each. I understand that their logic is pretty simple, but it can help to catch some issues with interface changes, getters, etc.
  • Negative test cases, for example, when mint is called by not bootloader. balanceOf with dirty bytes. And maybe you can think about withdraw.

Apart from that, usually we have another test structure - a separate describe block per each external function. You can have a look at the contract deployer test, for example. IMHO, it makes the test more structured and readable. But if you have another opinion - I am open to discussing it.

test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@AntonD3 AntonD3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM! Left only a few small comments. And can you please add just some simple test for the totalSupply getter? Sorry, forgot about this getter during the previous review.

test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
test/L2EthToken.spec.ts Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants