Stop emitting invalid omni-github log paths - #6761
Conversation
GitHub artifact URLs are not artifact-relative log files. Omitting them from test result rows prevents log_path_not_found ingestion warnings.
There was a problem hiding this comment.
Isaac Lab Review Bot
Summary
This change stops emitting log_paths in converted omni-github test rows, since omni-github interprets that field as artifact-relative file paths rather than external URLs. The converter edit is a single-line deletion, and the corresponding test was renamed and updated to assert the key is absent. The fix matches the described ingestion warning and is low risk.
Architecture impact
Confined to the CI helper under .github/actions/upload-omni-github-test-results/. No runtime or public API surface is affected, and JUnit/comparison-image artifact uploads remain unchanged.
Test coverage
The updated regression test passes non-empty artifact URLs and asserts log_paths is absent from the emitted row, which would fail against the previous implementation (the key was always present). The dict-equality assertion in the crash/timeout test also pins the exact row schema. Coverage is adequate; the only gap is that nothing exercises the now-unused URL plumbing.
Implementation verdict
Minor fixes needed. Posted 1 actionable finding inline.
Automated comment-only review; human maintainers own approval decisions.
| junit_log_url, | ||
| comparison_images_url, | ||
| ] | ||
| assert "log_paths" not in rows[0] |
There was a problem hiding this comment.
🔵 Suggestion — Artifact URL inputs are now dead plumbing and the assertion is narrow
After dropping log_paths from the row, convert_junit still builds the log_paths list from junit_log_url/comparison_images_url, _convert_testcase still accepts an unused log_paths parameter, and both --junit-log-url and --comparison-images-url remain required=True on the CLI while feeding nothing. A future caller will reasonably assume those flags are wired up. Consider removing the unused parameter and list construction in junit_to_omni_github_results.py (making the CLI flags optional or dropping them alongside the action YAML that passes them). Independently, this assertion only pins the key name for the first row — reintroducing the URLs under a different key, or in additional rows, would still pass. Broadening it makes the test sensitive to the actual invariant.
| assert "log_paths" not in rows[0] | |
| assert all("log_paths" not in row for row in rows) | |
| result_text = (output_dir / _RESULT_PATH).read_text(encoding="utf-8") | |
| assert junit_log_url not in result_text | |
| assert comparison_images_url not in result_text |
Greptile SummaryRemoves invalid external artifact URLs from converted omni-github test rows.
Confidence Score: 5/5The PR appears safe to merge with no actionable defects identified. The result schema permits omission of Important Files Changed
Reviews (1): Last reviewed commit: "Stop emitting invalid omni-github log pa..." | Re-trigger Greptile |
| test_type: str, | ||
| group_id: str, | ||
| retries: int, | ||
| log_paths: list[str], |
There was a problem hiding this comment.
I would remove log path collection entirely if omnighithub is only for observability and does not persist the logs
Description
omni-github interprets
log_pathsas files relative to the uploadedtest-result artifact. Isaac Lab was putting external GitHub artifact URLs in
that field, so ingestion emitted
log_path_not_foundwarnings for everyconverted test row.
This change omits
log_pathsfrom converted rows. The existing JUnit andcomparison-image artifact uploads remain unchanged.
To view an example of warning logs:
Processed_with_warningsin the "Processing Evidence" table.Type of change
Validation
pytest .github/actions/upload-omni-github-test-results/test_junit_to_omni_github_results.py— 4 passed
hook could not run because
git-lfsis not installed on the developmenthost.
Checklist
source/was changed, so no changelog fragment isrequired.