Repository navigation
Tests: the redactor scaling check holds on a busy machine (#4788) - #4819
Merged
Merged
Conversation
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
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.
Fixes #4788
Why
PgSettingRedactorTests.LongInputWithNoSeparator_ScalesNearLinearlyNotCatastrophically(trailingAssignment: True)failed once in a fullDarling.Testsrun on a busy machine. The class passed alone right after. The test timesPgSettingRedactor.Redactat 2,000 and 32,000 characters (minimum of 5 repeats each) and fails when the larger pass takes 64 times as long as the smaller one, with the smaller time floored at 1 ms. So the bar is about 64 ms for the large pass, and when the rest of a 17,000-test suite keeps the machine busy for all 5 repeats, the minimum can pass 64 ms without any regex problem.What changes
The first alternative from the issue: the class runs with no other test class running.
PgSettingRedactorTimingCollection:[CollectionDefinition("pg-setting-redactor-timing", DisableParallelization = true)], the same shape asPgFileSettingsStaticsCollection.PgSettingRedactorTestscarries[Collection("pg-setting-redactor-timing")], with a doc paragraph saying why.Nothing else changes. Both assertions stay exactly as they were: the exact-output checks, and the scaling check (bar of 64 times, 1 ms floor on the baseline, 5 repeats, minimum of the repeats). No retry was added and nothing was widened.
The original failure came from a loaded machine and is not reproduced here. This change removes the concurrency it came from and does not loosen the bar, which is what keeps a real quadratic pattern failing (below).
RED plant
The issue asks that a pattern that backtracks badly but finishes under the 100 ms regex timeout still fails the scaling assertion, before and after the change. The plant was a temporary extra pattern in
PgSettingRedactor.Redact,(?:pass)+=y(a run ofpasswith a suffix that never matches, so every start position rescans the rest of the run). It changes no output, so the exact-output checks still pass under it and only the timing check can catch it. It measured 66 to 103 ms at 32,000 characters and under 1.3 ms at 2,000, so at the large size it sits right at the 100 ms match timeout (a timeout is caught insideRedact, which then masks the value, so the measured time stays near 100 ms either way). The plant is removed and is not in the diff.Before the change (plant in, class not yet in the collection): both theory rows failed.
After the change (plant still in, class in the collection), 4 runs. Each run failed the scaling assertion in one of the two rows:
The other row passed in each of those runs. A quadratic pattern that must stay under the 100 ms timeout can only sit in a narrow band above the 64 ms bar, so this plant lands on either side of it from run to run. The bar itself did not move.
Test plan
PgSettingRedactorTeststogether withDocCommentHygiene: 216 total, 0 failed.DARLING_TEST_PG), and the runner reported 1 as not run (it did not say which).The full runs above were made from this branch only. The two sibling test-fix PRs (#4815 and #4817) branch from dev on their own, so those runs do not include their changes.
CHANGELOG
SECTION: None