feat(sanity): check sample data file only if it is valid#269
Conversation
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
There was a problem hiding this comment.
Pull request overview
This PR updates sanity reference checks for SampleData file paths so that file existence is only validated for records that are marked is_valid=True, reducing false “file not found” failures for invalid/ignored records.
Changes:
- REF201: gate
SampleData.filenameexistence checks behindrecord["is_valid"]. - REF202: gate
SampleData.info_filenamepresence/existence checks behindrecord["is_valid"].
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| t4_devkit/sanity/reference/ref201.py | Skip file existence checks for invalid SampleData records. |
| t4_devkit/sanity/reference/ref202.py | Skip info_filename checks for invalid SampleData records. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Reason(f"File not found: {record['filename']}") | ||
| for record in records | ||
| if not data_root.joinpath(record["filename"]).exists() | ||
| if record.get("is_valid", True) and not data_root.joinpath(record["filename"]).exists() |
There was a problem hiding this comment.
This comprehension can raise a KeyError if a SampleData record is missing the required "filename" key (e.g., malformed JSON): the filter only checks is_valid, but the element expression and exists() path both index record["filename"]. Since checkers are run without exception handling, this can crash the entire sanity run. Consider using record.get("filename") (or similar) and either skip missing filenames or emit a dedicated Reason for missing/empty filename before calling joinpath().
| Reason(f"File not found: {record['filename']}") | ||
| for record in records | ||
| if not data_root.joinpath(record["filename"]).exists() | ||
| if record.get("is_valid", True) and not data_root.joinpath(record["filename"]).exists() |
There was a problem hiding this comment.
The new is_valid guard changes REF201 behavior (invalid records should no longer trigger missing-file errors), but there’s no test asserting that an is_valid=False record with a missing/invalid filename is ignored. Adding a focused unit test would prevent regressions and also ensure the checker doesn’t error on intentionally-invalid records.
| Reason(f"File not found: {record['filename']}") | ||
| for record in records | ||
| if not data_root.joinpath(record["filename"]).exists() | ||
| if record.get("is_valid", True) and not data_root.joinpath(record["filename"]).exists() |
There was a problem hiding this comment.
For readability/consistency with REF202, consider formatting this condition across multiple lines (or extracting is_valid/filename to locals). The current single-line comprehension is harder to scan and maintain as more guards get added.
| if record.get("is_valid", True) | ||
| and record.get("info_filename") is not None | ||
| and not data_root.joinpath(record["info_filename"]).exists() |
There was a problem hiding this comment.
The new is_valid guard changes REF202 behavior (invalid records should be ignored), but there’s no test asserting that an is_valid=False record with a missing/nonexistent info_filename does not produce a failure. Adding a unit test for this case would prevent future regressions.
What
This pull request updates the file existence checks in two sanity reference modules to ensure that only records marked as valid are considered. This prevents unnecessary file-not-found errors for records that are not valid.
Sanity check improvements:
t4_devkit/sanity/reference/ref201.pyso that the file existence check only applies to records whereis_validisTrue.t4_devkit/sanity/reference/ref202.pyto include theis_validcheck before verifying the presence ofinfo_filenameand its existence.