Better tests

[rndquu]

Let's make tests better.

Coverage

Right now if you run forge coverage --fork-url https://cloudflare-eth.com you get the following:

File	% Lines	% Statements	% Branches	% Funcs
Total	27.42% (773/2819)	28.04% (882/3145)	11.86% (154/1298)	46.35% (216/466)

So code coverage could be better for a stablecoin project.

Test naming

Also there is no single template for test naming, check out these 3 different test naming styles:

• https://github.com/ubiquity/ubiquity-dollar/blob/development/packages/contracts/test/dollar/ERC20Ubiquity.t.sol#L104
• https://github.com/ubiquity/ubiquity-dollar/blob/development/packages/contracts/test/dollar/Staking.t.sol#L95
• https://github.com/ubiquity/ubiquity-dollar/blob/development/packages/contracts/test/dollar/core/CreditNFTManager.t.sol#L101

testPermit_ShouldRevert_IfSignatureIsInvalid vs testCannotDeployEmptyAddress vs test_setExpiredCreditNFTConversionRate

So new developers who come to the project can't understand what style to choose.

ERC4626 tests

We've got the following tests for the ERC4626 contract: https://github.com/ubiquity/ubiquity-dollar/blob/development/packages/contracts/test/dollar/ERC4626.t.sol

There are a few issues with those tests:

1. Basically it is testing a mock contract https://github.com/ubiquity/ubiquity-dollar/blob/development/packages/contracts/src/dollar/mocks/MockERC4626.sol
2. ERC4626 is already tested by the openzeppelin and its community
3. Those tests run really long on a forked environment (~15min). This is a tedious experience for new devs. Also it moves us closer to Github actions CI limits of 6 hours per repo per day.

Proposed tasks

1. Refactor tests to maintain a single naming convention

Refactor tests using the following convention test[MethodName]_[ExpectedResult]_[Condition]

Examples:

• https://github.com/cryptexfinance/contracts/blob/master/test/HardETHVaultHandler.t.sol#L74
• https://github.com/ubiquity/ubiquity-dollar/blob/development/packages/contracts/test/dollar/ERC20Ubiquity.t.sol#L104

2. Remove tests for ERC4624

Remove:

• https://github.com/ubiquity/ubiquity-dollar/blob/development/packages/contracts/test/dollar/ERC4626.t.sol
• https://github.com/ubiquity/ubiquity-dollar/blob/development/packages/contracts/src/dollar/mocks/MockERC4626.sol

3. Add CI build badge

Add CI build badge at the top of the https://github.com/ubiquity/ubiquity-dollar/blob/development/README.md with the current CI build status (success or fail)

4. Add test coverage badge

Add test coverage badge with % of lines covered by tests at the top of the https://github.com/ubiquity/ubiquity-dollar/blob/development/README.md

5. Add CI action that fails on decreased test coverage

When user creates a PR to the development branch there should be a github action that checks that code coverage is not decreased. If code coverage is decreased the CI action should fail.

6. Remove mocks, libs and tests from coverage report

Remove the following files from the test coverage report:

• src/dollar/libs/*
• src/dollar/mocks/*
• src/dollar/utils/*
• src/manager/libraries/*
• src/ubiquistick/MockUBQmanager.sol
• test/*

7. Add more tests for ERC1155SetUri

Add more tests for:

• src/dollar/ERC1155SetUri/ERC1155BurnableSetUri.sol
• src/dollar/ERC1155SetUri/ERC1155PausableSetUri.sol
• src/dollar/ERC1155SetUri/ERC1155SetUri.sol

Test coverage right now:

File	% Lines	% Statements	% Branches	% Funcs
src/dollar/ERC1155SetUri/ERC1155BurnableSetUri.sol	0.00% (0/4)	0.00% (0/4)	0.00% (0/4)	0.00% (0/2)
src/dollar/ERC1155SetUri/ERC1155PausableSetUri.sol	100.00% (2/2)	100.00% (2/2)	50.00% (1/2)	100.00% (1/1)
src/dollar/ERC1155SetUri/ERC1155SetUri.sol	56.52% (52/92)	54.39% (62/114)	25.00% (10/40)	66.67% (14/21)

8. Add more tests for Staking

Add more tests for:

• src/dollar/Staking.sol
• src/dollar/StakingShare.sol

Test coverage right now:

File	% Lines	% Statements	% Branches	% Funcs
src/dollar/Staking.sol	84.62% (110/130)	85.33% (128/150)	57.14% (16/28)	66.67% (16/24)
src/dollar/StakingFormulas.sol	100.00% (9/9)	100.00% (12/12)	100.00% (2/2)	100.00% (4/4)
src/dollar/StakingShare.sol	75.00% (30/40)	71.43% (30/42)	33.33% (2/6)	71.43% (10/14)

9. Add more tests for UbiquityChef and UbiquityFormulas

Add more tests for:

• src/dollar/UbiquityChef.sol
• src/dollar/UbiquityFormulas.sol

Test coverage right now:

File	% Lines	% Statements	% Branches	% Funcs
src/dollar/UbiquityChef.sol	88.41% (61/69)	88.16% (67/76)	61.11% (11/18)	100.00% (15/15)
src/dollar/UbiquityFormulas.sol	40.74% (11/27)	42.55% (20/47)	0.00% (0/2)	40.00% (2/5)

10. Add more tests for CreditRedemptionCalculator, DollarMintExcess and UbiquityCreditToken

Add more tests for:

• src/dollar/core/CreditRedemptionCalculator.sol
• src/dollar/core/DollarMintExcess.sol
• src/dollar/core/UbiquityCreditToken.sol

Test coverage right now:

File	% Lines	% Statements	% Branches	% Funcs
src/dollar/core/CreditRedemptionCalculator.sol	0.00% (0/9)	0.00% (0/14)	0.00% (0/2)	0.00% (0/3)
src/dollar/core/DollarMintExcess.sol	0.00% (0/30)	0.00% (0/37)	0.00% (0/2)	0.00% (0/4)
src/dollar/core/UbiquityCreditToken.sol	0.00% (0/2)	0.00% (0/3)	100.00% (0/0)	0.00% (0/1)

11. Add more tests for ubiquistick: LP, SimpleBond and UAR

Add more tests for:

• src/ubiquistick/LP.sol
• src/ubiquistick/SimpleBond.sol
• src/ubiquistick/UAR.sol

Test coverage right now:

File	% Lines	% Statements	% Branches	% Funcs
src/ubiquistick/LP.sol	0.00% (0/1)	0.00% (0/1)	100.00% (0/0)	0.00% (0/1)
src/ubiquistick/SimpleBond.sol	84.91% (45/53)	86.44% (51/59)	44.44% (8/18)	85.71% (12/14)
src/ubiquistick/UAR.sol	0.00% (0/1)	0.00% (0/1)	100.00% (0/0)	0.00% (0/1)

12. Add more tests for UbiquiStick and UbiquiStickSale

Add more tests for:

• src/ubiquistick/UbiquiStick.sol
• src/ubiquistick/UbiquiStickSale.sol

Test coverage right now:

File	% Lines	% Statements	% Branches	% Funcs
src/ubiquistick/UbiquiStick.sol	42.31% (11/26)	39.29% (11/28)	0.00% (0/12)	57.14% (8/14)
src/ubiquistick/UbiquiStickSale.sol	0.00% (0/13)	0.00% (0/15)	0.00% (0/6)	0.00% (0/6)

[zgorizzo69]

always in favor of more tests overall your proposition makes sense
just keep in mind that very often trying to achieve 100% makes no sense as it requires much more work to achieve the last percent compared to the benefits that we could have
here you focus on unit testing and we can also add more integration testing that will target full flow instead of just one function of one contract

[0x4007]

This proposal looks well thought out. Unfortunately I can't weigh too much of an opinion because I am not experienced with Solidity development. Generally speaking more tests are good but I understand and agree with diminishing returns as per @zgorizzo69 comment. We just need to use our best judgement on how much coverage we will have.

Once we have the input from all the @ubiquity/bounty-masters @ubiquity/development Solidity developers lets convert these to separate issues. @rndquu you can add the label that represents your best time estimate for each bounty once we create the issues (e.g. <1 day, <1 week) you can also set all of them to priority 0 or 1.

@rndquu I invited you so you can set labels https://github.com/orgs/ubiquity/teams/development/members

[0x4007]

ERC4626 tests

We should write a test for the vulnerability found. We might even be able to fork one from one of the linked GitHub links.

#183 (comment)

[hashedMae]

Agree that there's no need to test contracts from libraries. If it's from somewhere like OpenZeppelin or Uniswap we can generally trust that they've already been thoroughly tested. If it's from somewhere that's more of an unknown then we should be thoroughly reviewing the contracts before implementing them as a library. Permissioned functions fall under this and we don't need to test for when an account without the proper role calls one.

Tests need to follow a common naming convention and we need to account for each possible state (ie just deployed is Zero State)
Event emits should be covered in their relevant tests (if testing a deposit function, should listen for the event within that test rather than writing a separate test function just for the event)

We have test helpers that deploy the contracts in the proper order, for both mainnet forks and offline, all that's needed is to inherit from them and call super.setUp()

The pattern of Contract(manager.contractAddress).function() is utterly ridiculous and such a hot mess. Just call the contracts as they are in the test helpers.

We need to ensure that we're testing both when a function should succeed, and when it should fail. Some functions have multiple fail states and we need to test for each one.

If a feature is added or altered then the relevant PR shouldn't be merged unless it includes adequate testing.

edit: Our tests are also running way too long, I imagine it's worse for anyone that's using a computer that's more than 3 years old. Anything we can do to optimize that will be a positive, and eliminating unnecessary tests is a good first step.

[0xcodercrane]

This proposal looks good. Thank you for your writing @rndquu. I definitely agree with creating a stable stablecoin project. Test coverage is very important.

1. Refactor tests to maintain a single naming convention.
   I like this naming convention as well as mae's state consideration. so we can combine both.

2. Remove tests for ERC4624.
   Agree that we don't need to write unit tests of contracts from other libraries.

3. Add CI build badge ✔️

4. Add test coverage badge ✔️

5. Add CI action that fails on decreased test coverage 🟡. This might need more research to make the task simple.

6. Remove mocks, libs and tests from coverage report ✔️
   7 - 12. Add more tests ✔️

If I had to say the general perspective of this discussion, I might say its not urgent rather than implementing general architecture to unblock UI testing env setup.

1/ Complete a deploy script PR which is still ongoing by @hashedMae .
2/ Refactor frontend interacting with the deploy script.
There are some references on frontend for ABIs and contract addresses deployed. We need to update them as needed.
So whenever we make changes on contract side, it should be deployed on local chain and UI needs to connect with the contracts deployed on that commit. This is SUPER important because we can unblock ui testing PRs once its done.

@rndquu would you please take a deep dive on this if you have bandwidth?

[hashedMae]

I removed the mock tests cause I was sick of them slowing down testing.

[hashedMae]

If we adopt the testing guidelines from the Foundry Book that should help us optimize for what's important and provides a robust naming style.