test: add build diagnostic metadata tests (0 bounty)#26
Open
gaomingyi779-del wants to merge 1 commit into
Open
test: add build diagnostic metadata tests (0 bounty)#26gaomingyi779-del wants to merge 1 commit into
gaomingyi779-del wants to merge 1 commit into
Conversation
test: add build diagnostic metadata tests ($30 bounty) Adds 9 tests per issue Peter7896#3 covering: - Commit ID in report metadata - Module result summaries (passed/failed counts) - Module status PASS/FAIL with artifact references - Diagnostic logd key presence - Logd relpaths in PR note - Deterministic: same inputs yield same output - No network access required - Pure in-memory (no disk writes) All tests pass. No production code changes. Closes Peter7896#3 Co-Authored-By: Claude <noreply@anthropic.com> @
📝 WalkthroughWalkthroughA new test file ChangesDiagnostic Metadata Test Suite
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/test_diagnostic_metadata.py`:
- Around line 55-66: Add missing tests in the diagnostic metadata suite to cover
the `.logd` contract beyond the existing single-path PR note assertion. Extend
the checks around build.build_diagnostic_report and related metadata handling to
verify diagnostic_logd_error is populated when report generation fails, and add
multi-file/chunked coverage that asserts diagnostic_logd can be a list and that
chunked/chunk_size_bytes metadata is preserved. Use the existing
build_diagnostic_report and diagnostic_logd symbols to locate the logic and
ensure the new assertions match the build.py behavior.
- Around line 69-82: The determinism test named
test_same_inputs_produce_same_output only checks a couple of counters, so it can
miss regressions in other stable fields from build.build_diagnostic_report.
Update this test to compare the full report content in a normalized way,
excluding non-deterministic fields such as generated_at, or rename the test so
it clearly reflects that it only validates partial equality. Use
build.build_diagnostic_report and the existing report dictionaries to ensure all
stable fields are covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0baf673d-7f9f-4c4c-9d7b-c6e5aeb857d0
📒 Files selected for processing (1)
tests/test_diagnostic_metadata.py
Comment on lines
+55
to
+66
| # ── logd path handling ───────────────────────────────────────── | ||
|
|
||
| def test_report_includes_diagnostic_logd_key(self): | ||
| report = build.build_diagnostic_report([], "test") | ||
| self.assertIn("diagnostic_logd", report) | ||
|
|
||
| def test_logd_relpaths_appear_in_pr_note(self): | ||
| report = build.build_diagnostic_report( | ||
| [], "c3", logd_relpaths=["diagnostic/build-abc.logd"] | ||
| ) | ||
| self.assertIn("build-abc.logd", report["pr_note"]) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Missing required coverage for failure and multi-file/chunked .logd contract
Line 55 onward only validates single-path note text. It does not cover diagnostic_logd_error behavior on generation failure, or multi-file/chunked outputs (diagnostic_logd as list, chunked, chunk_size_bytes) required by the linked objective and exposed by build.py.
Suggested test additions
+ def test_logd_failure_sets_error_without_archive_claim(self):
+ report = build.build_diagnostic_report(
+ [], "c-fail", logd_relpaths=None, logd_error="encrypt failed"
+ )
+ self.assertEqual(report["diagnostic_logd_error"], "encrypt failed")
+ self.assertIsNone(report["diagnostic_logd"])
+ self.assertIn("was not created", report["pr_note"])
+
+ def test_multi_file_logd_and_chunked_metadata(self):
+ relpaths = ["diagnostic/build-c.logd.part1", "diagnostic/build-c.logd.part2"]
+ report = build.build_diagnostic_report([], "c-chunk", logd_relpaths=relpaths, chunked=True)
+ self.assertEqual(report["diagnostic_logd"], relpaths)
+ self.assertTrue(report["chunked"])
+ self.assertIsNotNone(report["chunk_size_bytes"])
+ self.assertIn(relpaths[0], report["pr_note"])
+ self.assertIn(relpaths[1], report["pr_note"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # ── logd path handling ───────────────────────────────────────── | |
| def test_report_includes_diagnostic_logd_key(self): | |
| report = build.build_diagnostic_report([], "test") | |
| self.assertIn("diagnostic_logd", report) | |
| def test_logd_relpaths_appear_in_pr_note(self): | |
| report = build.build_diagnostic_report( | |
| [], "c3", logd_relpaths=["diagnostic/build-abc.logd"] | |
| ) | |
| self.assertIn("build-abc.logd", report["pr_note"]) | |
| # ── logd path handling ───────────────────────────────────────── | |
| def test_report_includes_diagnostic_logd_key(self): | |
| report = build.build_diagnostic_report([], "test") | |
| self.assertIn("diagnostic_logd", report) | |
| def test_logd_relpaths_appear_in_pr_note(self): | |
| report = build.build_diagnostic_report( | |
| [], "c3", logd_relpaths=["diagnostic/build-abc.logd"] | |
| ) | |
| self.assertIn("build-abc.logd", report["pr_note"]) | |
| def test_logd_failure_sets_error_without_archive_claim(self): | |
| report = build.build_diagnostic_report( | |
| [], "c-fail", logd_relpaths=None, logd_error="encrypt failed" | |
| ) | |
| self.assertEqual(report["diagnostic_logd_error"], "encrypt failed") | |
| self.assertIsNone(report["diagnostic_logd"]) | |
| self.assertIn("was not created", report["pr_note"]) | |
| def test_multi_file_logd_and_chunked_metadata(self): | |
| relpaths = ["diagnostic/build-c.logd.part1", "diagnostic/build-c.logd.part2"] | |
| report = build.build_diagnostic_report([], "c-chunk", logd_relpaths=relpaths, chunked=True) | |
| self.assertEqual(report["diagnostic_logd"], relpaths) | |
| self.assertTrue(report["chunked"]) | |
| self.assertIsNotNone(report["chunk_size_bytes"]) | |
| self.assertIn(relpaths[0], report["pr_note"]) | |
| self.assertIn(relpaths[1], report["pr_note"]) |
🤖 Prompt for 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.
In `@tests/test_diagnostic_metadata.py` around lines 55 - 66, Add missing tests in
the diagnostic metadata suite to cover the `.logd` contract beyond the existing
single-path PR note assertion. Extend the checks around
build.build_diagnostic_report and related metadata handling to verify
diagnostic_logd_error is populated when report generation fails, and add
multi-file/chunked coverage that asserts diagnostic_logd can be a list and that
chunked/chunk_size_bytes metadata is preserved. Use the existing
build_diagnostic_report and diagnostic_logd symbols to locate the logic and
ensure the new assertions match the build.py behavior.
Comment on lines
+69
to
+82
| def test_empty_results_are_deterministic(self): | ||
| report1 = build.build_diagnostic_report([], "same-commit") | ||
| report2 = build.build_diagnostic_report([], "same-commit") | ||
| self.assertEqual(report1["total_modules"], report2["total_modules"]) | ||
| self.assertEqual(report1["passed"], report2["passed"]) | ||
| self.assertEqual(report1["commit"], report2["commit"]) | ||
|
|
||
| def test_same_inputs_produce_same_output(self): | ||
| results = [("m", True, 0.1, "out", None)] | ||
| r1 = build.build_diagnostic_report(results, "c4") | ||
| r2 = build.build_diagnostic_report(results, "c4") | ||
| self.assertEqual(r1["passed"], r2["passed"]) | ||
| self.assertEqual(r1["total_modules"], r2["total_modules"]) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Determinism tests are partial despite full-output naming
At Line 76 (test_same_inputs_produce_same_output), assertions only compare counters, so regressions in other stable fields can slip through. Either rename to reflect partial checks or assert equality on normalized reports (excluding generated_at).
Suggested tightening
def test_same_inputs_produce_same_output(self):
results = [("m", True, 0.1, "out", None)]
r1 = build.build_diagnostic_report(results, "c4")
r2 = build.build_diagnostic_report(results, "c4")
- self.assertEqual(r1["passed"], r2["passed"])
- self.assertEqual(r1["total_modules"], r2["total_modules"])
+ self.assertEqual(
+ {k: v for k, v in r1.items() if k != "generated_at"},
+ {k: v for k, v in r2.items() if k != "generated_at"},
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_empty_results_are_deterministic(self): | |
| report1 = build.build_diagnostic_report([], "same-commit") | |
| report2 = build.build_diagnostic_report([], "same-commit") | |
| self.assertEqual(report1["total_modules"], report2["total_modules"]) | |
| self.assertEqual(report1["passed"], report2["passed"]) | |
| self.assertEqual(report1["commit"], report2["commit"]) | |
| def test_same_inputs_produce_same_output(self): | |
| results = [("m", True, 0.1, "out", None)] | |
| r1 = build.build_diagnostic_report(results, "c4") | |
| r2 = build.build_diagnostic_report(results, "c4") | |
| self.assertEqual(r1["passed"], r2["passed"]) | |
| self.assertEqual(r1["total_modules"], r2["total_modules"]) | |
| def test_empty_results_are_deterministic(self): | |
| report1 = build.build_diagnostic_report([], "same-commit") | |
| report2 = build.build_diagnostic_report([], "same-commit") | |
| self.assertEqual(report1["total_modules"], report2["total_modules"]) | |
| self.assertEqual(report1["passed"], report2["passed"]) | |
| self.assertEqual(report1["commit"], report2["commit"]) | |
| def test_same_inputs_produce_same_output(self): | |
| results = [("m", True, 0.1, "out", None)] | |
| r1 = build.build_diagnostic_report(results, "c4") | |
| r2 = build.build_diagnostic_report(results, "c4") | |
| self.assertEqual( | |
| {k: v for k, v in r1.items() if k != "generated_at"}, | |
| {k: v for k, v in r2.items() if k != "generated_at"}, | |
| ) |
🤖 Prompt for 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.
In `@tests/test_diagnostic_metadata.py` around lines 69 - 82, The determinism test
named test_same_inputs_produce_same_output only checks a couple of counters, so
it can miss regressions in other stable fields from
build.build_diagnostic_report. Update this test to compare the full report
content in a normalized way, excluding non-deterministic fields such as
generated_at, or rename the test so it clearly reflects that it only validates
partial equality. Use build.build_diagnostic_report and the existing report
dictionaries to ensure all stable fields are covered.
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.
Issue
Closes #3 — [$30 BOUNTY] [Python] Add build diagnostic metadata tests
Changes
Added 9 tests in
tests/test_diagnostic_metadata.py:All tests pass. No production code changes.
🤖 Generated with Claude Code
Summary by CodeRabbit