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

feat: increase test coverage #135

Open
wants to merge 202 commits into
base: feature/yneigen
Choose a base branch
from

Conversation

johnnyonline
Copy link
Member

No description provided.




}
Copy link
Contributor

Choose a reason for hiding this comment

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

these tests shouldn't be deleted, not sure what was the intention here

Copy link
Member Author

Choose a reason for hiding this comment

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

blunder. added in last commit

@danoctavian
Copy link
Contributor

danoctavian commented Jul 29, 2024

Let's rebase this branch (yneigen-increase-test-coverage) on top of the base branch (feature/yneigen) to resolve conflicts

@johnnyonline johnnyonline force-pushed the yneigen-increase-test-coverage branch from 9838f34 to e41c794 Compare July 31, 2024 17:10
@@ -352,9 +352,9 @@ contract YnETHScenarioTest10 is IntegrationBaseTest, YnETHScenarioTest3 {
// The SelfDestructSender contract is created with the amountToSend and immediately self-destructs,
// sending its balance to the target address.
address(new SelfDestructSender{value: amountToSendViaSelfDestruct}(target));

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally if you don't want to change a file just revert it back to the version on the base branch, so there's no random changes in it


// should fail when not on the pause whitelist
ynEigenToken.approve(user2, amount);
vm.expectRevert(bytes4(keccak256(abi.encodePacked("TransfersPaused()"))));
Copy link
Contributor

Choose a reason for hiding this comment

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

please use expectRevert with TransfersPaused.selector as in the other tests for consistency

Copy link
Member Author

Choose a reason for hiding this comment

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

}
}

contract YnLSDScenarioTest2 is ynEigenIntegrationBaseTest {
Copy link
Contributor

@danoctavian danoctavian Aug 2, 2024

Choose a reason for hiding this comment

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

call it YnEigenScenarioTest2

Copy link
Member Author

Choose a reason for hiding this comment

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

import { Invariants } from "test/scenarios/Invariants.sol";
import { IERC20 } from "lib/openzeppelin-contracts/contracts/interfaces/IERC20.sol";
import { TestAssetUtils } from "test/utils/TestAssetUtils.sol";
import {Invariants} from "./Invariants.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

the identation in this file is messed up, it uses 8 spaces intead of 4

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm not sure what you're referring to here

Copy link
Contributor

Choose a reason for hiding this comment

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

I had this issue as well @johnnyonline... I had to change my cursor/vscode settings to use spaces, instead of tabs. Might be the cause of this one for you too

vm.stopPrank();
}

function test_ynLSD_Scenario_2_Unpause() public {
Copy link
Contributor

@danoctavian danoctavian Aug 2, 2024

Choose a reason for hiding this comment

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

change all places to have ynEigen

Copy link
Member Author

Choose a reason for hiding this comment

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

}

// | src/ynEIGEN/ynEigen.sol | 85.25% (52/61) | 87.67% (64/73) | 100.00% (20/20) | 75.00% (12/16)
Copy link
Contributor

@danoctavian danoctavian Aug 2, 2024

Choose a reason for hiding this comment

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

it's good you measured this but let's not put it in the file, just leave it in the description of the PR.

It becomes a chore to keep updating it as we add code or add tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

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