test: add regression tests for Live2D review blockers#350
Conversation
- backup-clean --json actually deletes old backups - flatten copies motion Sound assets - save_model uses atomic write (tempfile + os.replace) - auto_backup deduplicates on content, not wall-clock time - save_model cleans up temp files on error
zyzly0705
requested review from
omerarslan0,
yuh-yang and
zhangxilong-43
as code owners
github-actions
Bot
added
the
existing-cli-fix
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9bf3a8fa1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Simulate backup-clean --keep 2 (non-dry-run, non-json path deletes inline) | ||
| to_delete = backups[2:] | ||
| for f in to_delete: | ||
| f.unlink() |
There was a problem hiding this comment.
This test claims to cover backup-clean --json, but in the scenario where the JSON branch regresses back to reporting files without deleting them, it would still pass because the test deletes to_delete itself and never invokes the CLI with --json backup-clean --keep 2. That leaves the actual blocker untested; use CliRunner against the command and then assert only two backup files remain.
Useful? React with 👍 / 👎.
The previous test deleted files manually, which would pass even if the CLI regressed. Now it invokes 'backup-clean --keep 2 --json' through CliRunner and asserts both disk state and JSON output.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71914884ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Same content -> should skip | ||
| result2 = auto_backup(sample_model3_file) | ||
| assert result2 is None |
There was a problem hiding this comment.
Because the second auto_backup() call happens immediately after the first one, an implementation that incorrectly skips backups only because the latest backup is younger than a time threshold would still pass this regression test for the wrong reason. To actually distinguish unchanged-content deduplication from age-based throttling, make the existing backup appear old enough (or patch the clock / sleep past the threshold) before calling auto_backup() with unchanged content.
Useful? React with 👍 / 👎.
| # Verify only 2 backups remain on disk | ||
| remaining = sorted(bdir.glob("*.model3.json"), reverse=True) | ||
| assert len(remaining) == 2, f"Expected 2 backups after cleanup, got {len(remaining)}" |
There was a problem hiding this comment.
This only checks that two backups remain, so a regression that sorts backups oldest-first before applying --keep 2 would delete the three newest backups and still pass with exactly two files left. Since the test creates five ordered backups, compare the remaining paths to the pre-clean backups[:2] or inspect the remaining JSON versions to verify that backup-clean kept the most recent backups, not just any two backups.
Useful? React with 👍 / 👎.
2 participants
Adds the regression tests requested in #313 review:
Soundaudio files and rewrites references to flat pathstempfile.mkstemp+os.replacefor atomic writesAll 20 tests pass (6 new + 14 existing).