Close gaps in the Atlas removal marking tests - #1292
Merged
Conversation
Three of the tests added with the Atlas marking in Netflix#1288 checked less than their names suggest. meterSurvivingTheCleanupPassIsNotMarked asserted that the meter kept active is present and unmarked, which is also exactly what a pass that removed nothing produces. It now asserts the other four are gone, and fails if the sweep is made a no-op. Nothing covered the replacement for a removed meter. That is the case the reader added in Netflix#1290 depends on: if a lookup ever returned an already marked instance, every update would resolve again forever instead of making progress. Added a test that the replacement is a different, unmarked instance. closeMarks only read the flag, so it said nothing about whether the meter was still registered afterwards. It now checks that too. The ordering the flag depends on, that the mark is set after the entry is gone, is only observable from inside markRemoved() and stays pinned by RemovableMeterMarkingTest; the comment says so rather than implying this test covers it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tests only, no production change. Three of the tests added with the Atlas marking in #1288 checked less than their names suggest.
A no-op sweep satisfied the survivor test
meterSurvivingTheCleanupPassIsNotMarkedasserted that the meter kept active is present and unmarked — which is also exactly the state a pass that removed nothing produces. It now asserts the other four are gone. Verified: makingAtlasRegistry.removeExpiredMeters()a no-op fails it, and did not before.Nothing covered the replacement for a removed meter
This is the case the reader added in #1290 depends on.
SwapMeter.get()now resolves again whenisRemoved()is true, so if a lookup ever returned an already-marked instance — a change that recycled removed meters, or reset the map without resetting the flag — every update would resolve forever instead of making progress, at a fullregistry.counter(id)each time. The existing tests only ever looked at meters that had been removed, never at their replacements.theReplacementForARemovedMeterIsNotMarkedsweeps the counter, confirms the original is marked, re-creates it, and asserts the replacement is a different unmarked instance.closeMarks read the flag and nothing else
It now also checks the meter is no longer registered after
close().Worth being precise about what that does and does not pin: it is an end-state check. Whether the mark is set after the entry is removed rather than before — the invariant
markRemoved()documents, fixed in #1287 — is only observable from insidemarkRemoved()itself, and stays pinned byRemovableMeterMarkingTest, which records what the registry could find at that moment. I confirmed that revertingcloseState()to the mark-then-clear form does not fail this test, so the comment now points at the test that does cover it instead of implying this one does.