🧪 Add test for invalid snapshot JSON manifest structure#5
Conversation
Added `test_read_manifest_returns_none_on_invalid_entry_format` to test that reading a snapshot manifest with broken JSON data (specifically missing required keys for `SnapshotEntry` causing a `TypeError`) safely returns `None`. Also corrected `tests/fixtures/sample_type_aliases.py` to use `TypeAlias` from `typing` for pre-3.12 alias detection tests. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds regression coverage for SnapshotManager.read_manifest() when the manifest JSON parses but can’t be converted into SnapshotEntry objects, and updates a type-alias fixture to properly represent “pre-3.12” alias syntax.
Changes:
- Add a test that exercises the
TypeErrorpath when manifest entries are missing requiredSnapshotEntryfields. - Update the type-alias fixture to use
X: TypeAlias = Yfor pre-3.12 style aliases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_snapshot.py | Adds a new test covering invalid manifest entry structure (TypeError during SnapshotEntry(**e)). |
| tests/fixtures/sample_type_aliases.py | Adjusts the fixture to use TypeAlias assignments for pre-3.12 style aliases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -19,14 +19,16 @@ | |||
| # Dummy usage of type aliases to keep static analyzers happy. | |||
| test_file_path: FilePath | None = None | |||
|
|
|||
| from typing import TypeAlias | |||
Added an exception in `ruff.toml` for `UP040` on `tests/fixtures/sample_type_aliases.py` to prevent the linter from complaining about the use of `TypeAlias` since it is intentionally used for backward-compatibility testing. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
| type ErrorMessage = str | ||
| type ConfigDict = dict[str, str | int | bool | list[str]] | ||
| type NamePair = tuple[str, str] | ||
| FilePath: TypeAlias = str | Path |
…3013802 Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
🎯 What: The testing gap addressed was testing the
TypeErrorexception path inSnapshotManager.read_manifest()when the JSON manifest is parsed but has an invalid structure forSnapshotEntryinstantiation.📊 Coverage: The scenario where a valid JSON file is parsed, but its contents lack the required keys (
source,stored) for entries, causing aTypeError, is now explicitly tested. Also fixed an existing test failure related to pre-3.12 type alias detection.✨ Result: Test coverage for
src/exportify/common/snapshot.pyis improved, achieving robust handling of invalid manifest structures and guaranteeingread_manifestaccurately returnsNone.PR created automatically by Jules for task 13016918876953013802 started by @bashandbone