makerdao / spells-mainnet

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

feat: standarizing chainlog changes [PoC] #375

Closed wei3erHase closed 6 months ago

wei3erHase commented 6 months ago

The objective of this PoC PR is to standarize common chainlog actions such as add, remove, and update, and have a way of skipping tests should none of these actions be present.

The PR adds a file name actions.sol, in which the crafter will have to implement the expected actions before starting to write the DssSpell contract, making the tests be executed only when these tests are present, avoiding using private / public logic to disable and enable tests arbitrarily, and adding more steps on the pe-checklist.

A separate test could be run in order to check that if the chainlog did have some change (such as change in length, or in any given address) and any of the addresses was not properly declared, the test would fail.

image
DaiFoundation-DevOps commented 6 months ago

CLA assistant check
All committers have signed the CLA.

wei3erHase commented 6 months ago

The crafter should be expected to add any given change to the chainlog in the shape of a struct: image

amusingaxl commented 6 months ago

Hey there, thanks for taking the time and submitting this PR. Not everyone wants to get into the bushes with MakerDAO spell tests.

The idea is not terrible, but honestly it brings little to no gain compared to what's suggested on #374.

While it might look cleaner at a first glance, we are still dependent on the developer to remember to make those changes. Putting it in the constructor or in the setUp function is not ideal because those are updated quite infrequently, so it's easier to overlook them.

We do appreciate the idea of making tests more declarative, and we do have some written that way (i.e.: testGeneral and the values in config.sol), however those are more useful in the context of checking the current state of the protocol as a whole, not incremental changes made to it.

As a rule of thumb, we avoid being clever when writing tests, because there's nothing we can use to test the tests! hl8fy We need to rely purely on humans to review tests, and we want to keep them as boring as possible, so there is no room for fuck-ups.

However, I believe we could leverage the skip(bool) trick to make test skipping more explicit, so thank you for that.

wei3erHase commented 6 months ago

Thanks for the thoughtful reply! 🥰 I do believe there are certain techniques to test the tests, i'll think a bit about that. I'm closing this as, as stated, there's little advantage to previous setup, but pushing #376 as I think it could be a bit more beneficial.