makerdao / spells-mainnet

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

Fix misleading comment on `testNewChainlogValues` #348

Closed amusingaxl closed 1 year ago

amusingaxl commented 1 year ago

Description

This is a maintenance change.

The issue stems from this comment on the spell test file.

TL;DR:


The problem was originally mentioned in #347. I'm copying over some of the arguments for context:

This change was first introduced here: https://github.com/makerdao/spells-mainnet/pull/339/files#diff-cd597c47b9a6e2bae57aab767ee9e3d5bb3d8edeb3a2248b4519b9af791cb468R302

This is not a directive to further crafters to not disable this test, it's simply describing what it's doing.

Looking past that, the full history of the archive has the updated comment, which is in line with every other test that can be disabled.

See:

Before that, the practice was to toggle between // make public to use and // make private to disable, but we abandoned that practice.

See:

This comment has been misinterpreted and carried over for the past 5 spells. The test is redundant in all those spells you mentioned. The chainlog version is already checked here.

In reality, the 5 last spells failed to do the proper clean up as per the checklist.

Then:

The only thing present in the test now is checking the chainlog version, which is already done in other test.

And also:

There are only 2 instances in which this was done like that:

Every single other spell in the archive which contains this test is either active (meaning there are new chainlog entries to test) or marked private.

[Fixes #351]

DaiFoundation-DevOps commented 1 year ago

CLA assistant check
All committers have signed the CLA.

hexonaut commented 1 year ago

I never changed that comment, and I don't see why it's there. You guys can certainly make that private if there isn't new chainlog values.

amusingaxl commented 1 year ago

I managed to trace it back to Goerli actually:

https://github.com/makerdao/spells-goerli/blob/bd3c2de0b0527a79691aa2b2ba22a04536a42c28/archive/2023-01-24-DssSpell/DssSpell.t.sol#L210-L218

The issue is that Goerli currently does not check for the version in src/test/config.sol the same way mainnet does.

hexonaut commented 1 year ago

They should be synced.

amusingaxl commented 1 year ago

Sync'ing on spells-goerli#209.