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

Fix Migration Test Definitions #865

Merged
merged 2 commits into from
Dec 11, 2024
Merged

Fix Migration Test Definitions #865

merged 2 commits into from
Dec 11, 2024

Conversation

nlordell
Copy link
Collaborator

The migration tests are defining describe blocks asyncronously within an it block. AFAIU, this is a no-no and causes ugly test formatting. This PR adjusts the test definitions so that they belong to the correct parent describe block (one per migration version).

Test formatting before:
  addOwner
    ✔ should add owner and change threshold

  enableModule
    ✔ should enabled module and be able to use it

  multiSend
    ✔ execute multisend via delegatecall

  fallbackHandler
    ✔ should be correctly set

  execTransaction
    ✔ should be able to transfer ETH (48ms)

  addOwner
    ✔ should add owner and change threshold

  enableModule
    ✔ should enabled module and be able to use it

  multiSend
    ✔ execute multisend via delegatecall

  fallbackHandler
    ✔ should be correctly set

  execTransaction
    ✔ should be able to transfer ETH (48ms)

  addOwner
    ✔ should add owner and change threshold

  enableModule
    ✔ should enabled module and be able to use it

  multiSend
    ✔ execute multisend via delegatecall

  fallbackHandler
    ✔ should be correctly set

  execTransaction
    ✔ should be able to transfer ETH (45ms)

  addOwner
    ✔ should add owner and change threshold

  enableModule
    ✔ should enabled module and be able to use it

  multiSend
    ✔ execute multisend via delegatecall

  fallbackHandler
    ✔ should be correctly set

  execTransaction
    ✔ should be able to transfer ETH (49ms)

  addOwner
    ✔ should add owner and change threshold

  enableModule
    ✔ should enabled module and be able to use it

  multiSend
    ✔ execute multisend via delegatecall

  fallbackHandler
    ✔ should be correctly set

  execTransaction
    ✔ should be able to transfer ETH (52ms)

  addOwner
    ✔ should add owner and change threshold

  enableModule
    ✔ should enabled module and be able to use it

  multiSend
    ✔ execute multisend via delegatecall

  fallbackHandler
    ✔ should be correctly set
Test formatting after:
  Upgrade from Safe 1.1.1
    execTransaction
      ✔ should be able to transfer ETH (50ms)
    addOwner
      ✔ should add owner and change threshold
    enableModule
      ✔ should enabled module and be able to use it
    multiSend
      ✔ execute multisend via delegatecall
    fallbackHandler
      ✔ should be correctly set

  Upgrade from Safe 1.2.0
    execTransaction
      ✔ should be able to transfer ETH (45ms)
    addOwner
      ✔ should add owner and change threshold
    enableModule
      ✔ should enabled module and be able to use it
    multiSend
      ✔ execute multisend via delegatecall
    fallbackHandler
      ✔ should be correctly set

  Upgrade from Safe 1.3.0
    execTransaction
      ✔ should be able to transfer ETH (52ms)
    addOwner
      ✔ should add owner and change threshold
    enableModule
      ✔ should enabled module and be able to use it
    multiSend
      ✔ execute multisend via delegatecall
    fallbackHandler
      ✔ should be correctly set

  Upgrade from Safe 1.3.0 L2
    execTransaction
      ✔ should be able to transfer ETH (50ms)
    addOwner
      ✔ should add owner and change threshold
    enableModule
      ✔ should enabled module and be able to use it
    multiSend
      ✔ execute multisend via delegatecall
    fallbackHandler
      ✔ should be correctly set

  Upgrade from Safe 1.4.1
    execTransaction
      ✔ should be able to transfer ETH (52ms)
    addOwner
      ✔ should add owner and change threshold
    enableModule
      ✔ should enabled module and be able to use it
    multiSend
      ✔ execute multisend via delegatecall
    fallbackHandler
      ✔ should be correctly set

  Upgrade from Safe 1.4.1 L2
    execTransaction
      ✔ should be able to transfer ETH (51ms)
    addOwner
      ✔ should add owner and change threshold
    enableModule
      ✔ should enabled module and be able to use it
    multiSend
      ✔ execute multisend via delegatecall
    fallbackHandler
      ✔ should be correctly set

The migration tests are defining `describe` blocks asyncronously within
an `it` block. AFAIU, this is a no-no and causes ugly test formatting.
This PR adjusts the test definitions so that they belong to the correct
parent `describe` block (one per migration version).
@nlordell nlordell requested review from a team, akshay-ap, mmv08 and remedcu and removed request for a team December 11, 2024 07:49
Copy link
Member

@akshay-ap akshay-ap left a comment

Choose a reason for hiding this comment

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

LGTM

@nlordell nlordell merged commit e9a75ac into main Dec 11, 2024
25 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants