search

makerdao / spells-mainnet

Staging repo for MakerDAO weekly executive spells
GNU Affero General Public License v3.0
107 stars 43 forks source link

Refactor: explicit skip & improve office-hours check & add missing error messages #379

Closed amusingaxl closed 4 months ago

amusingaxl commented 6 months ago

Following up some suggestions made in #375, we can leverage the skip(bool) cheatcode to explicitly skip tests instead of disabling them by making them private.

The advantage is that the skipped tests will actually show in the execution output: image

This makes it harder to miss test that should be executed but are not. Skipped tests should also be part of the reviewer checklist.

amusingaxl commented 4 months ago

I would suggest to open a PR with process changes in makerdao/pe-checklists before merging this one (especially regarding the differentiation between private and public skipped)

Will do, just wanted to get feedback on the structure here first so there's not a lot of back and forth in the checklist.

SidestreamColdMelon commented 4 months ago

If we would have linter, would be great to forbid private modifier in DssSpell.t.sol from now on to avoid confusion.

amusingaxl commented 4 months ago

If we would have linter, would be great to forbid private modifier in DssSpell.t.sol from now on to avoid confusion.

Last time I checked, it was not possible to have different linter rules for different files using solhint. That might have changed recently.