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

Cleanup test names / scopelint fix #22

Merged
merged 10 commits into from
Jan 8, 2024
Merged

Conversation

jferas
Copy link
Contributor

@jferas jferas commented Dec 11, 2023

Cleaned up test contracts and how they are organized and named for better compatibility with "scopelint spec" command.

test/RadworksGovernor.t.sol Outdated Show resolved Hide resolved
test/RadworksGovernor.t.sol Outdated Show resolved Hide resolved
test/helpers/RadworksGovernorTest.sol Show resolved Hide resolved
@@ -17,7 +16,8 @@ contract Constants {
uint256 constant MAX_REASONABLE_TIME_PERIOD = 302_400; // 6 weeks assume a 12 sec block time

// we have not yet deployed the Radworks Bravo Governor
address constant DEPLOYED_BRAVO_GOVERNOR = 0x1111111111111111111111111111111111111111;
// @dev: this is a placeholder address and will be replaced with the deployed address
address constant DEPLOYED_BRAVO_GOVERNOR = address(0x0);

Choose a reason for hiding this comment

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

(arbitrary comment)

  • When looking at the Scopelint spec of PoolTogether I noticed a couple test cases were missing in Radworks.

│ ├── Old Governor Cannot Send E T H After Proposal Succeeds
│ ├── Old Governor Sends Token After Proposal Is Defeated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: 2d0828d

@jferas jferas force-pushed the test-cleanup-scopelint-spec-fix branch 2 times, most recently from 360bb5b to d2f8877 Compare December 12, 2023 19:18
}

function testFuzz_CanGrantPauserOnDrips(address _newPauser) public {
// assummeNotTimelock(_newPauser);

Choose a reason for hiding this comment

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

Should this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, done: 2d0828d

@jferas
Copy link
Contributor Author

jferas commented Dec 13, 2023

This commit e41b463 moved test functions from RadworksGovernorAlpha.t.sol (and deleted it) into RadworksGovernor.t.sol so that the test functions would be reported on by scopelint spec which expects the tests to reside in a test file with the same base name as the contract to be tested.

test/Constants.sol Outdated Show resolved Hide resolved
test/RadworksGovernor.t.sol Show resolved Hide resolved
test/RadworksGovernor.t.sol Show resolved Hide resolved
@jferas jferas force-pushed the test-cleanup-scopelint-spec-fix branch 2 times, most recently from 5321783 to f20777f Compare December 28, 2023 16:12
@jferas jferas changed the base branch from drips-admin-tests to main December 28, 2023 16:12
@wildmolasses wildmolasses self-requested a review January 8, 2024 20:57
@jferas jferas force-pushed the test-cleanup-scopelint-spec-fix branch from f20777f to b0e85b2 Compare January 8, 2024 21:27
Copy link

github-actions bot commented Jan 8, 2024

Coverage after merging test-cleanup-scopelint-spec-fix into main will be

76.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   RadworksGovernor.sol73.91%75%69.23%76%101, 126–127, 135, 138, 152, 203, 84

@jferas jferas merged commit 6ddeca0 into main Jan 8, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants