Cut tests that can't fail and make the weak ones catch regressions - #235
Merged
Merged
Conversation
An audit of all 630 tests found 25 that guard nothing: they assert a constant equals itself, print a benchmark, or repeat a stronger test on the same code path. They are removed. Eighteen more guarded real behavior but could not fail. The largest: the .ica round-trip tests imported and exported through copies of the production code kept inside the test file, so a broken importer still passed. They now go through loadFromFilePath and zipStrategy against a throwaway Hive library, every fixture round-trips, and corrupt files must fail without saving. Skipping migration in the real importer now fails the fixture test; before, it passed. The rest: assertions too loose to fail, expected values computed by the code under test, setup that never reached the branch named in the title, and a Windows-only provider path that could not tell its two entry points apart. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The video size-target and zip Paranoia tests were not covered elsewhere: the 10/20 MiB targets set the re-encode threshold, and only the Paranoia test round-trips lineup and deleted Paranoias. Both are back. The dense cone query keeps its ray and edge counters without the benchmark loop. The fixture round-trip now reimports the exported .ica itself and compares image attachments, so an exporter that drops images fails. Imports and exports write their images inside the test's temp library, and the Hive group deletes its temp library instead of leaking one per run. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 39 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (30)
💤 Files with no reviewable changes (13)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis PR updates tests across strategy archives, map and placement geometry, widgets, and providers. It adds direct coordinate and geometry assertions, exercises strategy imports and exports through a provider-backed harness, and changes or removes several existing test checks. ChangesTest suite updates
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Other Merge Risk: ⚪ Minimal · up to The tests strengthen production archive round trips and behavioral assertions without an established workflow regression. Mergeable subject to normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The fixture matrix compared the second export with the first, so content lost on the first import went unnoticed. Every page and every placed element in each fixture must now come through import by id. If opening the harness fails partway, it closes Hive, removes its path_provider mock and deletes its folder before rethrowing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@greptileai review |
An agent tile takes its size from its icon, so until the image decodes the tile is 0x0 and a real tap lands on whatever sits underneath. CI hit that race once the test stopped calling onTap directly. The harness now precaches both icons before settling. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
greptile-apps
Bot
dismissed
their stale review
October 1, 2026 22:42
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
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.
An audit of every test (630
test/testWidgetscalls across 116 files) for ones that can't fail or only repeat a stronger test. Answer: 24 are useless (3.8%), and 19 more guarded real behavior but were built so they couldn't catch a regression. This cuts the first group and rewrites the second. 630 → 607 tests.The one that mattered
strategy_integrity_test.dart, the .ica round-trip suite that guards the library, never ran the app's importer or exporter. It used_importStrategyFromDecodedand_buildExportPayload, copies of production code kept in the test file, and the copies had already drifted from production (theme palette resolution, the version guard, image loading). A broken real importer still passed.The tests now go through
loadFromFilePathandzipStrategyagainst a throwaway Hive library:test/fixtures/strategy_integrity/imports to the current version. The exported.icaitself is then reimported and must re-export the same JSON and the same image attachments (canvas-compatibility-v97.icacarries 7 PNGs).mapDatamust throw and leave the library empty.%TEMP%(I found 166).Mutation checks against the real code, reverted afterwards:
strategy_provider.dartmigrateLegacyDatain_importStrategyFilezipStrategystops packing image filesCut (24)
-(rotation ?? 0), and two single-fixture round-trips the fixture matrix now covers.Rewritten (19)
screenPositionForWidgetsnaps circles to whole pixels on purpose. The test now states that tolerance.decodeWorldGzip's footer check; the screenshot failure test fails before coordinates are entered.Review
Astra (gpt-6-astra) checked every cut and every rewrite against the code. Two cuts were wrong and are restored in the second commit: the 10/20 MiB video targets set the re-encode threshold, and the zip Paranoia test was the only one round-tripping lineup and deleted Paranoias. Astra also caught the image gap and the temp-folder leak above.
Kept on purpose: the direct version-guard test for current/older versions. Every real-path import test uses a current-version file, so nothing else proves an older .ica gets past the guard.
For Dara
strategy_folder_import_test.dart"manifest zip/directory apply failure does not fall back to legacy import" require the "Manifest Root" folder to still exist after a failed import. That locks a half-finished write in as expected behavior, which goes against "fail loudly without saving". Untouched here; worth deciding whether that's intended.Verification
flutter test(full suite, Windows): 624 passed, 3 skipped (native SVG, needsICARUS_SVG_NATIVE_LIBRARY) on the first commit. Second-round files: 71 passed.flutter analyzeclean on touched files.dart format-clean on main were formatted.🤖 Generated with Claude Code
Summary by CodeRabbit
No outstanding findings block merging.
Summary
The PR strengthens tests for strategy import/export and widget interactions. The two previously reported test concerns are fixed, and no findings remain outstanding.
Reviews (3) · Last reviewed commit: "Wait for agent icons before tapping thei..."