refactor: add instructions on how to deal with the skipped modifier

makerdao · Feb 9, 2024 · 0d2c4f7 · 0d2c4f7
1 parent b0a283c
commit 0d2c4f7
Show file tree

Hide file tree

Showing 4 changed files with 16 additions and 6 deletions.
diff --git a/spell/spell-crafter-goerli-workflow.md b/spell/spell-crafter-goerli-workflow.md
@@ -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 setting adding the `skipped` modifier
     * [ ] Add commented notes
       * [ ] e.g. `// Insert new collateral integration tests here`
     * [ ] Keep commented tests (e.g. `testOSMs`, `testMedianizers`)
@@ -110,7 +110,9 @@ 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 is enabled, it MUST NOT have the `skipped` modifier
+    * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier
   * [ ] Tests PASS via `make test`
 * [ ] Open PR & Add Reviewers
 * [ ] Iterate until polls are ended and exec doc is confirmed

diff --git a/spell/spell-crafter-mainnet-workflow.md b/spell/spell-crafter-mainnet-workflow.md
@@ -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 setting adding the `skipped` modifier
     * [ ] Add commented notes
       * [ ] e.g. `// Insert new collateral integration tests here`
     * [ ] Keep commented tests (e.g. `testOSMs`, `testMedianizers`)
@@ -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
   * [ ] Tests PASS via `make test`
 * [ ] Open PR & Add Reviewers
 * [ ] Iterate until polls are ended and exec doc is confirmed

diff --git a/spell/spell-reviewer-goerli-checklist.md b/spell/spell-reviewer-goerli-checklist.md
@@ -243,7 +243,10 @@ Spell Actions (Per Exec Sheet):
 * [ ] 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 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
+  * [ ] Check if every test with the `skipped` modifier should be skipped
   * [ ] 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_

diff --git a/spell/spell-reviewer-mainnet-checklist.md b/spell/spell-reviewer-mainnet-checklist.md
@@ -313,7 +313,10 @@
 * 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 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
+  * [ ] Check if every test with the `skipped` modifier should be skipped
   * [ ] 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_