tests: pin _enableExperimentalOptimizations = true via subclass of ModelStateLayoutTests#161
Open
tsushanth wants to merge 1 commit into
Open
Conversation
…delStateLayoutTests `MagazineLayout._enableExperimentalOptimizations` is documented in `MagazineLayout.swift` as "A temporary flag to enable safely testing some optimizations" (added in airbnb#156). The flag gates 35+ branches across `MagazineLayout.swift`, `SectionModel.swift`, and `ModelState.swift`, but no existing test exercises it. Flipping the default is risky without automated coverage of the optimized code paths. This change adds `ModelStateLayoutTestsWithExperimentalOptimizations`, which subclasses `ModelStateLayoutTests` and flips the flag in `setUp` (restoring `false` in `tearDown`). XCTest reruns every inherited test method against the optimized path. Any divergence between the optimized and legacy paths now fails a test. Mechanically: - Drop `final` from `ModelStateLayoutTests` so it can be subclassed (test-only change; production code unaffected) - Add a single 47-line subclass file that overrides `setUp` / `tearDown` - Wire the new file into the Xcode test target Verified locally: ModelStateLayoutTests (11 tests, flag=false) + ModelStateLayoutTestsWithExperimentalOptimizations (11 tests, flag=true) = 22 tests, 0 failures. This is a starting wedge — if accepted, the same pattern can extend to `ModelStateUpdateTests`, `ElementLocationFramePairsTests`, `LayoutStateTargetContentOffsetTests`, etc. Happy to follow up with those in a separate PR once you confirm the approach. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author
|
Filed a broader proposal at #162 that frames this PR as Phase 1 of a path to flipping the flag and eventually removing it. Cross-linking so the PR and the proposal sit in each other's context. |
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.
Why
MagazineLayout._enableExperimentalOptimizationsis documented inMagazineLayout.swiftas "A temporary flag to enable safely testing some optimizations" (added in #156). The flag now gates 35+ branches acrossMagazineLayout.swift,SectionModel.swift, andModelState.swift, but no existing test exercises it.Without automated coverage of the optimized code paths, flipping the default to
trueis risky — any divergence between the optimized and legacy paths can land silently.What
Adds
ModelStateLayoutTestsWithExperimentalOptimizations, a subclass ofModelStateLayoutTeststhat flips the flag insetUpand restores it intearDown. XCTest reruns every inherited test method against the optimized path, so any divergence between the two paths now fails a test.Verified locally on iOS Simulator (iPhone 17 Pro, iOS 26.4):
ModelStateLayoutTests(11 tests, flag =false) +ModelStateLayoutTestsWithExperimentalOptimizations(11 tests, flag =true) = 22 tests, 0 failures.Mechanical changes:
finalfromModelStateLayoutTestsso it can be subclassed (test-only file; production code untouched).setUp/tearDown.Scope
Intentionally narrow — this is the smallest wedge that validates the approach. If you're happy with the pattern, the same subclass-and-flip approach extends naturally to:
ModelStateUpdateTestsElementLocationFramePairsTestsLayoutStateTargetContentOffsetTestsModelStateEmptySectionLayoutTestsModelStateInitiallSetUpTestsHappy to push a follow-up PR with those once you confirm the direction. I kept this one small so it's easy to review and so any feedback on the pattern lands once, not five times.
What this enables
Once test coverage of the optimized path exists, flipping the default of
_enableExperimentalOptimizationstotruebecomes a one-line PR that you can confidently land. Eventually the flag can be removed entirely.