Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed test case in eth.test.ts #567

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

justmert
Copy link
Contributor

@justmert justmert commented Jan 7, 2025

Fixed expect and now it fails.

$ mocha --parallel tests/integration/eth.test.ts --timeout 30000 --bail


  Ether
    ✔ transfers ether on l2 (209ms)
    ✔ "EthBridger.approveGasToken" throws when eth is used as native/gas token
    1) deposits ether


  2 passing (12s)
  1 failing

  1) Ether
       deposits ether:

      balance failed to update after eth deposit
      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (tests/integration/eth.test.ts:139:12)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

Created the same issue before #407 but @spsjvc didn't approve it. I am one hundred percent sure that the below test case does not check anything meaningful.

The invalid test statement is in below:

expect(
initialInboxBalance.add(ethToDeposit).eq(finalInboxBalance),
'balance failed to update after eth deposit'
)

Here's why this is meaningless:

Please log below log statements after the test case:

console.log('Initial balance:', initialInboxBalance.toString())
console.log('Deposit amount:', ethToDeposit.toString())
console.log(
'Expected after deposit:',
initialInboxBalance.add(ethToDeposit).toString()
)
console.log('Actual final balance:', finalInboxBalance.toString())
console.log(
'Are they equal?:',
initialInboxBalance.add(ethToDeposit).eq(finalInboxBalance)
)

Initial balance: 0
Deposit amount: 200000000000000
Expected after deposit: 200000000000000
Actual final balance: 0
Are they equal?: false

As you can see even though they are not equal, the test still pass!

Btw, below statements also pass! Please check it out yourself.

expect(
false,
'balance failed to update after eth deposit'
)

expect(
true,
'balance failed to update after eth deposit'
)

The tests needs to be like below in order to actually check it:

expect(
initialInboxBalance.add(ethToDeposit).eq(finalInboxBalance),
'balance failed to update after eth deposit'
).to.be.true

@cla-bot cla-bot bot added the cla-signed label Jan 7, 2025
@CLAassistant
Copy link

CLAassistant commented Jan 7, 2025

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants