Test: Unit tests for Member Role #571

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

roxdanila merged 10 commits into nexus-v2 from test/member-roles

Jan 6, 2023

Contributor

MilGard91 commented Dec 13, 2022 •

edited

Loading

Context

Issue: #345

Changes proposed in this pull request

Added unit test to increase the coverage of MomberRoles.

Test plan

N/A

Checklist

Rebased the base branch
Attached corresponding Github issue
Prefixed the name with the type of change (i.e. feat, chore, test)
Performed a self-review of my own code
Followed the style guidelines of this project
Made corresponding changes to the documentation
Didn't generate new warnings
Didn't generate failures on existing tests
Added tests that prove my fix is effective or that my feature works

Review

When reviewing a PR, please indicate intention in comments using the following emojis:

🍰 = Nice to have but not essential.
💡 = Suggestion or a comment based on personal opinion
🔨 = I believe this should be changed.
🤔 = I don’t understand something, do you mind giving me more context?
🚀 = Feedback

MilGard91 requested a review from danoctavian

December 13, 2022 17:48

MilGard91 changed the title ~~Add test for Member Role~~ Test: Unit tests for Member Role

MilGard91 force-pushed the test/member-roles branch 2 times, most recently from 3c92d33 to 8725772 Compare

December 13, 2022 22:37

MilGard91 marked this pull request as ready for review

December 13, 2022 22:38

shark0der force-pushed the test/member-roles branch from 8725772 to 8d535bd Compare

December 14, 2022 08:53

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/changeAuthorized.js Outdated

    
                  await memberRoles.connect(governanceContracts[0]).changeAuthorized(Role.AdvisoryBoard, defaultSender.address);

                  const authorizedAddressAfter = await memberRoles.authorized(Role.AdvisoryBoard);

                  expect(authorizedAddressBefore).not.to.be.eq(defaultSender.address);

Contributor

danoctavian Dec 14, 2022

minor, but we've been spelling out eq as equal in other tests

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/changeAuthorized.js Outdated

    
                  const { defaultSender, governanceContracts } = this.accounts;

                  const authorizedAddressBefore = await memberRoles.authorized(Role.AdvisoryBoard);

                  await memberRoles.connect(governanceContracts[0]).changeAuthorized(Role.AdvisoryBoard, defaultSender.address);

Contributor

danoctavian Dec 15, 2022

let's use something else other than defaultSender.address

When you write tests, look to make the values arbitrary for generality, whatever that thing that you are switching to is. Arbitrary within constraints of course (eg. arbitrary member)

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/changeAuthorized.js Show resolved Hide resolved

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/changeAuthorized.js Outdated

    
                  ).to.be.reverted;

                  await expect(

                    memberRoles.connect(defaultSender).changeAuthorized(Role.AdvisoryBoard, governanceContracts[0].address),

                  ).to.not.reverted;

Contributor

danoctavian Dec 15, 2022

to.not.be.reverted

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/changeMaxABCount.js Show resolved Hide resolved

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/changeMaxABCount.js Outdated

    
                  const { governanceContracts } = this.accounts;

                  const maxABCountBefore = await memberRoles.maxABCount();

                  await memberRoles.connect(governanceContracts[0]).changeMaxABCount(2);

Contributor

danoctavian Dec 15, 2022

use named constants

const newMaxABCount = 2;

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/setup.js Outdated

    
                }

                // Setting AB Member

                const [abMember] = accounts.advisoryBoardMembers;

                await master.enrollMember(abMember.address, 2);

Contributor

danoctavian Dec 15, 2022 •

edited

Loading

use the right constant instead of the 2. Role.whatever

same as below

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/stateManagement.js Outdated Show resolved Hide resolved

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/stateManagement.js Outdated

    
                  } = this.accounts;

                  const [memberAddress, isActive] = await memberRoles.memberAtIndex(Role.Member, 0);

                  expect(member.address).to.be.equal(memberAddress);

Contributor

danoctavian Dec 15, 2022

this is the other way around.

expect(memberAddress).to.be.equal(member.address)

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/stateManagement.js Outdated

    
                it('should clear storage', async function () {

                  const { memberRoles } = this.contracts;

                  await expect(memberRoles.storageCleanup()).to.not.reverted;

Contributor

danoctavian Dec 15, 2022

to.not.be.reverted

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/stateManagement.js Outdated

    
                  const isMemberABMember = await memberRoles.checkRole(member.address, Role.AdvisoryBoard);

                  expect(isMemberMember).to.be.equal(true);

                  expect(isMemberNonMember).to.be.equal(true);

Contributor

danoctavian Dec 15, 2022

this is odd why is this true?

Contributor

roxdanila Jan 5, 2023

danoctavian reviewed

View reviewed changes

test/unit/MemberRoles/stateManagement.js Outdated

    
                  const isABMemberABMember = await memberRoles.checkRole(advisoryBoardMember.address, Role.AdvisoryBoard);

                  expect(isABMemberMember).to.be.equal(true);

                  expect(isABMemberNonMember).to.be.equal(true);

Contributor

danoctavian Dec 15, 2022

also odd that it's true

Contributor

danoctavian Dec 15, 2022

may be "correct" but warrants at least a comment to clarify why since it looks counterintuitive

Contributor

roxdanila Jan 5, 2023

danoctavian suggested changes

View reviewed changes

Contributor

danoctavian left a comment

some changes necessary

roxdanila force-pushed the test/member-roles branch from 8d535bd to b58f53e Compare

December 15, 2022 11:44

roxdanila linked an issue

that may be closed by this pull request

MemberRoles unit tests: Increase coverage to 100% #345

Closed

roxdanila force-pushed the test/member-roles branch from b58f53e to cc8946d Compare

December 15, 2022 13:35

MilGard91 force-pushed the test/member-roles branch from a16c0dd to 7d86896 Compare

December 15, 2022 17:38

danoctavian approved these changes

View reviewed changes

Contributor

danoctavian left a comment

LGTM now

danoctavian reviewed

View reviewed changes

contracts/mocks/MasterMock.sol Outdated

    
                enum Role {

                  NonMember,

                  Member,

                  AdvisoryBord,

Contributor

danoctavian Dec 20, 2022

mispelling here

Contributor

shark0der Dec 22, 2022

Let's use Unassigned (capitalization) and AdvisoryBoard (missing "a").

danoctavian approved these changes

View reviewed changes

Contributor

danoctavian left a comment

lgtm but let's fix that mispelling as well since we're here

shark0der reviewed

View reviewed changes

contracts/mocks/MemberRoles/MRMockCover.sol Outdated Show resolved Hide resolved

shark0der reviewed

View reviewed changes

lib/constants.js Outdated Show resolved Hide resolved

shark0der reviewed

View reviewed changes

test/unit/Assessment/setup.js Show resolved Hide resolved

shark0der reviewed

View reviewed changes

test/unit/MemberRoles/changeDependentContractAddress.js Outdated

    
              const { InternalContractsIDs } = require('../utils').constants;

              const { expect } = require('chai');

              const { hex } = require('../../../lib/helpers');

              const { ZERO_ADDRESS } = require('../../../lib/constants');

Contributor

shark0der Dec 22, 2022

Use AddressZero from ethers.constants instead

shark0der reviewed

View reviewed changes

test/unit/MemberRoles/changeDependentContractAddress.js Outdated

Comment on lines 7 to 16

    
                before(async function () {

                  const { quotationData, memberRoles } = this.contracts;

                  const { governanceContracts, defaultSender } = this.accounts;

                  await quotationData.connect(governanceContracts[0]).setKycAuthAddress(defaultSender.address);

                  await memberRoles.connect(governanceContracts[0]).setKycAuthAddress(quotationData.address);

                });

                it('should change authorized address for the role', async function () {

                  const { memberRoles, master } = this.contracts;

Contributor

shark0der Dec 22, 2022

Given there's only one test here, move the contents of the before in the test itself

shark0der reviewed

View reviewed changes

test/unit/MemberRoles/setup.js Show resolved Hide resolved

MilGard91 force-pushed the test/member-roles branch 2 times, most recently from a100367 to bd9c68f Compare

January 4, 2023 15:44

roxdanila force-pushed the test/member-roles branch 2 times, most recently from 5caf4db to 08d767e Compare

January 5, 2023 11:57

roxdanila requested changes

View reviewed changes

Contributor

roxdanila left a comment

still open questions

roxdanila force-pushed the test/member-roles branch from 08d767e to 6f3fc10 Compare

January 5, 2023 13:39

shark0der force-pushed the test/member-roles branch from 6f3fc10 to b46fa6b Compare

January 5, 2023 15:42

MilGard91 and others added 9 commits

January 6, 2023 14:02


          Add test for Member Role

e57e33b


          Review changes:

672362a

- formatting and code style fixes
- added test for `changeDependentContractAddress`


          fix: Roles discrepancy in MemberRoles and MasterMock

5a4e136


          Fix: UnAssigned naming

8d0b82e


          Review changes:

26667d6

- zero address from ethers
- typo fixes
- removing unnecessary before


          Replace UnAssigned with Unassigned

002aefd


          Simplify QD deploy and padding of the role name/description strings

249e9ee


          Remove oz import and implement revert using chai's expect

0a1845a


          Use deep equal for arrays and other stylistic fixes

3c875c9

shark0der force-pushed the test/member-roles branch from f0a9f3d to 3c875c9 Compare

January 6, 2023 12:02


          Remove unused variable

6539b25

roxdanila approved these changes

View reviewed changes

roxdanila merged commit 9b86db3 into nexus-v2

roxdanila deleted the test/member-roles branch

January 6, 2023 13:12

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

Labels

None yet