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.

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

Refactor: explicitly skip disabled tests #21

Merged
merged 4 commits into from
Feb 12, 2024
Merged

Refactor: explicitly skip disabled tests #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d2c4f7
refactor: add instructions on how to deal with the `skipped` modifier
amusingaxl Feb 9, 2024
2989365
fix: typos flagged by code review
amusingaxl Feb 12, 2024
5940684
refactor: improve items related to skipped tests
amusingaxl Feb 12, 2024
74f36c2
Update spell/spell-crafter-goerli-workflow.md
amusingaxl Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions spell/spell-crafter-goerli-workflow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ PR: https://github.com/makerdao/spells-goerli/pull/<TODO>
* [ ] Disable specific tests IF Not Used (e.g. `testCollateralIntegrations`, `testNewChainlogValues`, `testNewIlkRegistryValues`, ...)
* [ ] Remove spell-specific part
* [ ] Keep setup
* [ ] Disable by setting visibility to `private`
* [ ] Skip by adding the `skipped` modifier
* [ ] Add commented notes
* [ ] e.g. `// Insert new collateral integration tests here`
* [ ] Keep commented tests (e.g. `testOSMs`, `testMedianizers`)
Expand Down Expand Up @@ -110,7 +110,8 @@ PR: https://github.com/makerdao/spells-goerli/pull/<TODO>
* [ ] Add new ChainLog Address in `addresses_goerli.sol` (e.g. Collateral Onboarding)
* [ ] Run Tests `make test` or `make test match=<test_name>` to inspect debug traces
* [ ] Ensure Good Coverage
* [ ] Ensure every test function is declared as public if enabled or private if disabled
* [ ] Ensure every test function is declared as `public`
* [ ] IF the test needs to run, it MUST NOT have the `skipped` modifier; OTHERWISE, it MUST have `skipped` modifier
* [ ] Tests PASS via `make test`
* [ ] Open PR & Add Reviewers
* [ ] Iterate until polls are ended and exec doc is confirmed
Expand Down
6 changes: 4 additions & 2 deletions spell/spell-crafter-mainnet-workflow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ Repo: https://github.com/makerdao/spells-mainnet
* [ ] Disable specific tests IF Not Used (e.g. `testCollateralIntegrations`, `testNewChainlogValues`, `testNewIlkRegistryValues`, ...)
* [ ] Remove spell-specific part
* [ ] Keep setup
* [ ] Disable by setting visibility to `private`
* [ ] Skip by adding the `skipped` modifier
* [ ] Add commented notes
* [ ] e.g. `// Insert new collateral integration tests here`
* [ ] Keep commented tests (e.g. `testOSMs`, `testMedianizers`)
Expand Down Expand Up @@ -117,7 +117,9 @@ Repo: https://github.com/makerdao/spells-mainnet
* [ ] Add new ChainLog Address in `addresses_mainnet.sol` (e.g. Collateral Onboarding)
* [ ] Run Tests `make test` or `make test match=<test_name>` to inspect debug traces
* [ ] Ensure Good Coverage
* [ ] Ensure every test function is declared as public if enabled or private if disabled
* [ ] Ensure every test function is declared as `public`
* [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier
* [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier
Comment on lines +121 to +122
Copy link
Contributor

Choose a reason for hiding this comment

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

Please then also change it in 3 other places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed this... Let me create a quick PR again.

* [ ] Tests PASS via `make test`
* [ ] Open PR & Add Reviewers
* [ ] Iterate until polls are ended and exec doc is confirmed
Expand Down
17 changes: 10 additions & 7 deletions spell/spell-reviewer-goerli-checklist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,15 +240,18 @@ Spell Actions (Per Exec Sheet):
* [ ] Ensure every spell variable is declared as `public`/`internal`
* [ ] Ensure every spell variable is used in the spell (no unused variables)
* [ ] Spell actions match the corresponding [Exec Sheet](https://docs.google.com/spreadsheets/d/1w_z5WpqxzwreCcaveB2Ye1PP5B8QAHDglzyxKHG3CHw)
* [ ] Tests
* [ ] Ensure each spell action has sufficient test coverage
_List actions for which coverage was checked here_
* [ ] Ensure every test function is declared as public if enabled or private if disabled
* Tests
* [ ] Ensure that the `DssExecLib.address` file is not being modified by the spell PR
* [ ] Check all CI tests are passing as at the latest commit
_Insert most recent commit hash where CI was passing_
* [ ] Ensure every test function is declared as `public`
* [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier
* [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier
* [ ] Ensure each spell action has sufficient test coverage
_List actions for which coverage was checked here_
* [ ] Ensure that any other env variable does not affect execution of the tests (for example, by inspecting the output of `printenv | grep "FOUNDRY_\|DAPP_"`)
* [ ] Check all tests are passing locally using `make test`
* [ ] Ensure that only `ETH_RPC_URL` is being used from env (i.e. no `match`, `block` or similar are active in your env)
* [ ] Ensure every test listed in the _coverage_ item above is present in the logs with the `[PASS]` prefix.

```
_Insert your passing local tests here_
Expand Down Expand Up @@ -280,12 +283,12 @@ _Insert your passing local tests here_
* [ ] Archive matches `src`
* [ ] `make diff-archive-spell` for current date or or `date="YYYY-MM-DD" make diff-archive-spell` (date as per cast timestamp)
* [ ] Ensure date corresponds to target Exec Sheet date
* [ ] Tests
* Tests
* [ ] Ensure that the `DssExecLib.address` file is not being modified by the spell PR
* [ ] Check all CI tests are passing as at the latest commit
_Insert most recent commit hash where CI was passing_
* [ ] Ensure that only `ETH_RPC_URL` is being used from env (i.e. no `match`, `block` or similar are active in your env)
* [ ] Check all tests are passing locally using `make test`
* [ ] Ensure that only `ETH_RPC_URL` is being used from env (i.e. no `match`, `block` or similar are active in your env)

```
_Insert your passing local tests here_
Expand Down
13 changes: 8 additions & 5 deletions spell/spell-reviewer-mainnet-checklist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,16 +311,19 @@
* [ ] Where addresses are fetched from the `ChainLog`, the variable name must match the value of the ChainLog key for that address (e.g. `MCD_VAT` rather than `vat`), except where the archive pattern differs from this pattern (e.g. MKR)
* [ ] Spell actions match the corresponding Exec Doc
* Tests
* [ ] Ensure each spell action has sufficient test coverage
_List actions for which coverage was checked here_
* [ ] Ensure every test function is declared as public if enabled or private if disabled
* [ ] Ensure that the `DssExecLib.address` file is not being modified by the spell PR
* [ ] Check all CI tests are passing as at the latest commit
_Insert most recent commit hash where CI was passing_
* [ ] Ensure every test function is declared as `public`
* [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier
* [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier
* [ ] Ensure each spell action has sufficient test coverage
_List actions for which coverage was checked here_
* [ ] Ensure that any other env variable does not affect execution of the tests (for example, by inspecting the output of `printenv | grep "FOUNDRY_\|DAPP_"`)
* [ ] Check all tests are passing locally using `make test`
* [ ] Ensure every test listed in the _coverage_ item above is present in the logs and with the `[PASS]` prefix.

```bash
```
_Insert your local test logs here_
```

Expand Down Expand Up @@ -360,7 +363,7 @@ _Insert your local test logs here_
* [ ] Ensure that any other env variable does not affect execution of the tests (for example, by inspecting the output of `printenv | grep "FOUNDRY_\|DAPP_"`)
* [ ] Check all tests are passing locally using `make test`

```bash
```
_Insert your local test logs here_
```

Expand Down
Loading