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

refactor: standarize testing for dai and mkr actions [PoC] #376

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wei3erHase
Copy link
Contributor

@wei3erHase wei3erHase commented Dec 18, 2023

This PR adds another PoC, in the same rationale of #375, but for more common actions such as DAI and MKR streams, payments, and yanks. The ultimate goal is to reduce what developer needs to touch in DssSpell.t.sol, and move the common actions (such as payments) to a separate and more auditable testing file, while avoiding disabling and enabling tests that are crucial for the correct check on these actions.

The workflow checks (same as previous tests):

  • correct length of arrays (e.g. expected amounts of Payees)
  • correct sum of payments
  • adds the dates all together (and avoids repeating them for different scopes)

The advantages of this process rationale:

  • adds no smart-contract security overhead, other than a correct file import at testing
  • forces (by process) beneficiaries to be declared by name (and be added to addresses_deployers.sol)
  • a [SKIP] test is easier to detect than a non-existing test
  • less line modifications in sparsed lines of DssSpell.t.sol
  • standarize payments and vesting testing cleanup patterns

Given that previous spell has no MKR payments or yanks, the test output looks like this:
image

As you replied in #375, the rule of thumb is to make tests as simple as possible, i agree, there's little advantage in that one (as it touches the changelog, and perhaps needs other process), but perhaps this approach on very common tasks could be a bit more beneficial for the testing process.

@wei3erHase wei3erHase changed the title feat: dai and mkr actions [PoC] feat: standarize testing for dai and mkr actions [PoC] Dec 19, 2023
@wei3erHase wei3erHase changed the title feat: standarize testing for dai and mkr actions [PoC] refactor: standarize testing for dai and mkr actions [PoC] Dec 19, 2023
@@ -0,0 +1,131 @@
// SPDX-FileCopyrightText: © 2021 Dai Foundation <www.daifoundation.org>
Copy link
Contributor

Choose a reason for hiding this comment

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

My main issue with this refactor is that it introduces yet another file which needs to be modified and reviewed, potentially in every single spell.

While I get the separation of concerns in principle, I believe it does not work well with the nature of spell crafting. We try to follow the principle of co-location to reduce the already high cognitive overhead involved in the task.

So I believe there is little to no gain making these changes, it's just moving code around.

Copy link
Contributor Author

@wei3erHase wei3erHase Dec 19, 2023

Choose a reason for hiding this comment

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

Indeed, the point being that instead of having to check in 2023-11-29 changes in DssSpell.t.sol scattered in lines L405-423, L475-477, L523-526, L546-555 & L590-603, a reviewer should have to check only 1 file, to make contrast the data with the Exec Doc.

On a further development, that file could be cleanup more easily (instead of relying in checking previous cleanup patterns), resulting on less changes on the test file, making each change raise more alerts, rather than being used the reviewer to see changes.

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.

2 participants