Define the scope of spell tests

[amusingaxl]

Adding this as a placeholder so we can continue this discussion: https://github.com/makerdao/spells-mainnet/pull/417#discussion_r1691311398

[amusingaxl]

What SHOULD be in tests:

• Sanity checks on constructor params of new contracts
• Sanity checks on values that are initialized/updated by the spell (i.e. file())

What COULD be in tests:

• E2E happy path of new contracts being added to the system, depending on the risk level (TBD)

What SHOULD NOT be in tests:

• Full coverage for new contracts: it's expected that new contracts are properly reviewed and audited and therefore have enough coverage.
• Unlikely edge case scenarios: it's expected that those are well documented and properly tested in the original repository.

Special cases:

• Jobs in the Keeper Network are currently not audited because they mostly perform permissionless keeping in the protocol.
  • Usually jobs are quite simple to execute, so they could be at least E2E tested to ensure correctness.

[SidestreamColdMelon]

Agree in general, however:

What COULD be in tests:

This is not specific enough. I think our least responsibility is to ensure correct functioning of all systems after each spell. We don't write end-to-end tests for every single change (eg rate changes) only because either 1) we know there are no edge cases to hit there (or they otherwise checked within general tests / exec lib) or 2) end-to-end test is actually part of the general tests / required by the checklist (eg _checkIlkIntegration). But for every new contract (except subdaos) I would always advice to have end-to-end happy path coverage.

Tests outside of the happy path (like the one prompted this discussion) can be clearly marked optional even if specifically requested by a reviewer.

it's expected that those are well documented and properly tested in the original repository.

Of course it's not expected that our review will magically replace in-depth audit. But I think it's spell reviewers responsibility to ensure (instead of expecting) that new contracts ether exactly match audits or (if there are no audits) they can not affect core functionality (e.g. does not get auth on vat). Not directly related to the test coverage discussion, we might want to clearly define this fine line. Or, if formulated differently: clearly limit what non-audited or even not-well-tested contract can do in a spell.

[0x3phemeralsoul]

@SidestreamColdMelon please document the agreement on the Crafter / Reviewer checklists