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.

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

Improve AccessManager tests #4613

Merged
merged 63 commits into from
Oct 4, 2023
Merged

Improve AccessManager tests #4613

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
@frangio frangio changed the base branch from feat/access-manager to master September 19, 2023 14:50
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
test/access/manager/AccessManager.test.js Outdated Show resolved Hide resolved
test/access/manager/AccessManager.test.js Outdated Show resolved Hide resolved
test/access/manager/AccessManager.test.js Show resolved Hide resolved
test/access/manager/AccessManager.test.js Outdated Show resolved Hide resolved
@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
14 checks passed
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 <hadrien.croubois@gmail.com>
Co-authored-by: Francisco Giordano <fg@frang.io>
frangio pushed a commit that referenced this pull request Oct 5, 2023
@ernestognw @frangio
Improve AccessManager tests (#4613)
37f0b3b 
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco Giordano <fg@frang.io>
(cherry picked from commit baf0e91)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xVolosnikov 0xVolosnikov 0xVolosnikov left review comments

@Amxx Amxx Amxx approved these changes

@frangio frangio Awaiting requested review from frangio

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