Skip to content

Improve AccessManager tests #4613

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.

Sign up for GitHub

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

Jump to bottom
Merged
frangio merged 63 commits into from
Oct 4, 2023
Merged

Improve AccessManager tests #4613

frangio merged 63 commits into from
Oct 4, 2023

Conversation

@ernestognw
Copy link
Member

@ernestognw ernestognw commented Sep 18, 2023 •
edited
Loading

Reaches 100% coverage in AccessManager

Every test that either makes use of the _canCallExtended, _canCallSelf or canCall is split in two sections:

  • The restrictions tests: These only check the possible execution paths depending on the role and delays. They don't make any assertion other than expected errors.
  • The state tests: These are setup with enough permissions so the call succeeds and check for state changes or event emissions.

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Sep 18, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 6e397cc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ernestognw ernestognw changed the base branch from master to feat/access-manager September 18, 2023 22:08
@ernestognw ernestognw mentioned this pull request Sep 18, 2023
Merged
0xVolosnikov
0xVolosnikov reviewed Sep 19, 2023
View reviewed changes
@frangio frangio changed the base branch from feat/access-manager to master September 19, 2023 14:50
ernestognw
ernestognw commented Sep 21, 2023
View reviewed changes
@ernestognw ernestognw requested a review from frangio September 28, 2023 16:46
Amxx
Amxx previously approved these changes Sep 28, 2023
View reviewed changes
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

Solidity looks good.
Don't have bandwidth to review tests right now. Will take a look later (that is not a merge blocker)

@frangio frangio mentioned this pull request Oct 2, 2023
Merged
frangio
frangio reviewed Oct 4, 2023
View reviewed changes
@ernestognw ernestognw requested review from Amxx and frangio October 4, 2023 03:51
Amxx
Amxx reviewed Oct 4, 2023
View reviewed changes
Amxx
Amxx reviewed Oct 4, 2023
View reviewed changes
@ernestognw ernestognw requested a review from Amxx October 4, 2023 14:56
Amxx
Amxx approved these changes Oct 4, 2023
View reviewed changes
Copy link
Collaborator

@Amxx Amxx left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Everything LGTM.

Again, did no finish reviewing all the tests. The part I reviewed looks good. Contracts loog good.

I raised some comments that should be addressed in a separate PR (they are about the testing part, so don't affect the 5.0 release)

@frangio frangio merged commit baf0e91 into OpenZeppelin:master Oct 4, 2023
ernestognw added a commit to ernestognw/openzeppelin-contracts that referenced this pull request Oct 4, 2023
@ernestognw @Amxx @frangio
Improve AccessManager tests (OpenZeppelin#4613)
e96276a 
Co-authored-by: Hadrien Croubois <[email protected]>
Co-authored-by: Francisco Giordano <[email protected]>
frangio added a commit that referenced this pull request Oct 5, 2023
@ernestognw @Amxx @frangio
Improve AccessManager tests (#4613)
37f0b3b 
Co-authored-by: Hadrien Croubois <[email protected]>
Co-authored-by: Francisco Giordano <[email protected]>
(cherry picked from commit baf0e91)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Amxx Amxx Amxx approved these changes

@frangio frangio Awaiting requested review from frangio

+1 more reviewer

@0xVolosnikov 0xVolosnikov 0xVolosnikov left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ignore-changeset

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ernestognw @frangio @Amxx @0xVolosnikov