fix: stop snapshot tests from modifying source files - #2227
Open
bruschill wants to merge 5 commits into
Open
Conversation
bruschill
added
the
skip-docs-check
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSnapshot tests now require explicit recording. CI discovers and validates 38 snapshot suites on pull requests and pushes to ChangesSnapshot documentation validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant DocsWorkflow
participant SnapshotTests
participant Repository
PullRequest->>DocsWorkflow: trigger validation for main
DocsWorkflow->>SnapshotTests: run 38 discovered selectors
SnapshotTests->>Repository: compare or record references
DocsWorkflow->>Repository: inspect modified snapshot and screenshot paths
DocsWorkflow-->>PullRequest: report validation result
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MeshtasticTests/SwiftUIViewSnapshotTests.swift`:
- Around line 138-150: Update the shouldRecord branch in the snapshot
dimension-change flow to return immediately after pngData.write(to:
snapshotFile) succeeds. Preserve the existing error Issue.record and return for
failed writes, and keep the later Issue.record only for the non-recording path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 00ddc2a4-2c81-4cdb-8d42-7446858f5370
⛔ Files ignored due to path filters (5)
MeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/aboutMeshtastic.pngis excluded by!**/*.pngMeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/batteryPluggedIn.pngis excluded by!**/*.pngMeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/formattingToolbar.pngis excluded by!**/*.pngMeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/securityVersionNag.pngis excluded by!**/*.pngMeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/signalBLE_all.pngis excluded by!**/*.png
📒 Files selected for processing (5)
.github/workflows/docs-deploy.ymlMeshtasticTests/SwiftUIViewSnapshotTests.swiftscripts/copy-snapshots.shspecs/013-docs-release-versioning/contracts/script-contract.mdspecs/013-docs-release-versioning/research.md
💤 Files with no reviewable changes (2)
- specs/013-docs-release-versioning/contracts/script-contract.md
- scripts/copy-snapshots.sh
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.
What changed?
SNAPSHOT_RECORD=1before snapshot tests create or replace reference PNGs.aboutMeshtasticreference and moveformattingToolbarto test-only snapshots.EmptyView.copy-snapshots.sh.Why did it change?
A normal snapshot test run silently created missing PNGs and rewrote references when dimensions changed.
copy-snapshots.shalso deleted an unreferenced docs snapshot, creating a record-delete-record loop.The node-row snapshot fixtures had also been detached from SwiftData since the liveness guard in
98b5444e, so 19 tests rendered one-point-high empty views and rewrote their references.Fixes #2213.
How is this tested?
upstream/main: the test passed and created an untracked PNG.TEST_RUNNER_SNAPSHOT_RECORD=1records the two missing references.upstream/mainworktree: 19 one-point-height failures and 19 rewritten references.ackErrors.png.copy-snapshots.shcopied 99 referenced files without deleting an orphan sentinel.git diff --check, shell syntax checks, and workflow suite discovery. The workflow finds all 38 suites.Screenshots/Videos (when applicable)
New reference images:
MeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/aboutMeshtastic.pngMeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/formattingToolbar.pngChecklist
docs/user/ordocs/developer/, and updated accordingly (see copilot-instructions.md for the view → doc page mapping). No user-facing documentation change is needed; theskip-docs-checklabel is applied.Summary by CodeRabbit
Bug Fixes
Documentation
Tests