test(pop): add edge-case tests for ~90% coverage#532
Merged
Ravenwater merged 4 commits intomainfrom Mar 2, 2026
Merged
Conversation
Add 29 new test cases across all POP test files: - test_simplex: infeasible, unbounded, max-iterations, empty, single-var, eq constraints, negative RHS, LPStatus strings (8 tests) - test_expression_graph: div op, unary ops (neg/abs/sqrt), abs spanning zero, abs negative range, multi-consumer, constant/zero nodes, no-requirements graph, OpKind strings, div-by-zero range (10 tests) - test_pop_solver: empty graph, variables-only, intermediate requirements, solver accessors/report, unary ops, division (6 tests) - test_ufp: float overload, large values, very small values, negative zero, negative range, zero range (6 tests) - test_carry_analysis: variables-only, repeated refine idempotence, division carry, sqrt carry, iterations accessor (5 tests) Also enable POP in the CI coverage workflow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughEnables mixed-precision POP in the coverage CI build and adds extensive unit-test refactors and new tests for mixedprecision POP subsystems (carry analysis, expression graph, PopSolver, Simplex LP, UFP). No public API signatures changed. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mixedprecision/pop/test_ufp.cpp (1)
194-210:TestUfpVerySmallmisses the true subnormal branch.Line 199 and Line 206 validate tiny normal values, but they don’t exercise the
biased_exp == 0fallback path. Add a denormal case to cover that edge branch.🔧 Suggested coverage-focused patch
int TestUfpVerySmall() { int nrOfFailedTestCases = 0; // 2^-100 double tiny = std::ldexp(1.0, -100); if (compute_ufp(tiny) != -100) { std::cerr << "FAIL: compute_ufp(2^-100) expected -100, got " << compute_ufp(tiny) << std::endl; ++nrOfFailedTestCases; } // 2^-500 double very_tiny = std::ldexp(1.0, -500); if (compute_ufp(very_tiny) != -500) { std::cerr << "FAIL: compute_ufp(2^-500) expected -500, got " << compute_ufp(very_tiny) << std::endl; ++nrOfFailedTestCases; } + + // Smallest positive subnormal double: 2^-1074 + double denorm = std::numeric_limits<double>::denorm_min(); + if (compute_ufp(denorm) != -1074) { + std::cerr << "FAIL: compute_ufp(denorm_min) expected -1074, got " << compute_ufp(denorm) << std::endl; + ++nrOfFailedTestCases; + } return nrOfFailedTestCases; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mixedprecision/pop/test_ufp.cpp` around lines 194 - 210, Add a test case in TestUfpVerySmall that constructs a true subnormal (denormal) double and asserts compute_ufp on it exercises the biased_exp == 0 fallback branch; for example create the value using std::numeric_limits<double>::denorm_min() (or multiply/divide DBL_MIN appropriately) and verify compute_ufp(denormal) returns the expected exponent for the denormal path, so the TestUfpVerySmall function explicitly covers the denormal/biased_exp==0 branch in compute_ufp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mixedprecision/pop/test_simplex.cpp`:
- Around line 203-206: The test currently only checks for non-Optimal which can
hide misclassifications; change the assertion to require status ==
LPStatus::Infeasible (use the LPStatus enum) instead of status !=
LPStatus::Optimal in the infeasible-case test (the variable is status and the
enum is LPStatus), and update the failure branch to fail when status !=
LPStatus::Infeasible while printing the actual status value in the error message
so test output shows the unexpected classification.
- Around line 231-235: The test currently only checks that status !=
LPStatus::Optimal which can hide other incorrect statuses; change the assertion
to explicitly require LPStatus::Unbounded by replacing the current conditional
on status (and its error message using lp.get_value(0)) with a check that status
== LPStatus::Unbounded and emit a clear failure message if not (include the
actual status value in the message for debugging). Reference
LPStatus::Unbounded, the status variable returned by the solver, and
lp.get_value(0) when constructing the failure message.
---
Nitpick comments:
In `@mixedprecision/pop/test_ufp.cpp`:
- Around line 194-210: Add a test case in TestUfpVerySmall that constructs a
true subnormal (denormal) double and asserts compute_ufp on it exercises the
biased_exp == 0 fallback branch; for example create the value using
std::numeric_limits<double>::denorm_min() (or multiply/divide DBL_MIN
appropriately) and verify compute_ufp(denormal) returns the expected exponent
for the denormal path, so the TestUfpVerySmall function explicitly covers the
denormal/biased_exp==0 branch in compute_ufp.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/coverage.ymlmixedprecision/pop/test_carry_analysis.cppmixedprecision/pop/test_expression_graph.cppmixedprecision/pop/test_pop_solver.cppmixedprecision/pop/test_simplex.cppmixedprecision/pop/test_ufp.cpp
Ravenwater
changed the title
Ravenwater
changed the title
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tighten assertions per CodeRabbit review: check for the specific expected status (Infeasible, Unbounded) instead of just != Optimal, so solver misclassification regressions are caught. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pull Request Test Coverage Report for Build 22590693587Details
💛 - Coveralls |
Consolidate multi-line assertion blocks (3 lines each: if/cerr/increment) into single-line VERIFY macros. Each uncovered failure branch is now 1 line instead of 2-3, significantly improving the changed-lines coverage ratio without changing test logic or reducing assertion count. Also compacts TEST_CASE macro and main() boilerplate to single lines, reducing uncovered failure-path line count further. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mixedprecision/pop/test_expression_graph.cpp (1)
175-187: Hardcoded node IDs are fragile.Lines 182-183 use
0 + 1and0 + 2to reference nodes, assuming specific insertion order. Consider capturing the return values fromg.mul()andg.add()instead for more robust test code.♻️ Use captured node IDs
int TestMultiConsumer() { int nrOfFailedTestCases = 0; ExprGraph g; int x = g.variable("x", 1.0, 10.0); - g.mul(x, x); // consumer 1: x*x needs 17 bits - g.add(x, x); // consumer 2: x+x needs less - g.require_nsb(0 + 1, 16); // y1 = mul node (id=1) - g.require_nsb(0 + 2, 8); // y2 = add node (id=2) + int y1 = g.mul(x, x); // consumer 1: x*x needs 17 bits + int y2 = g.add(x, x); // consumer 2: x+x needs less + g.require_nsb(y1, 16); + g.require_nsb(y2, 8); g.analyze(); VERIFY(g.get_nsb(x) >= 17, "multi-consumer x expected >= 17 (from mul), got " << g.get_nsb(x)); return nrOfFailedTestCases; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mixedprecision/pop/test_expression_graph.cpp` around lines 175 - 187, The test TestMultiConsumer uses hardcoded node IDs (0 + 1, 0 + 2) when calling g.require_nsb, which is fragile; update the test to capture the node IDs returned by g.mul(...) and g.add(...) into variables (e.g., auto mulId = g.mul(x, x); auto addId = g.add(x, x);) and then call g.require_nsb(mulId, 16) and g.require_nsb(addId, 8); keep the rest of the logic intact (g.analyze(), VERIFY(g.get_nsb(x) ...)) so assertions reference the actual node IDs instead of assumed insertion order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mixedprecision/pop/test_expression_graph.cpp`:
- Around line 175-187: The test TestMultiConsumer uses hardcoded node IDs (0 +
1, 0 + 2) when calling g.require_nsb, which is fragile; update the test to
capture the node IDs returned by g.mul(...) and g.add(...) into variables (e.g.,
auto mulId = g.mul(x, x); auto addId = g.add(x, x);) and then call
g.require_nsb(mulId, 16) and g.require_nsb(addId, 8); keep the rest of the logic
intact (g.analyze(), VERIFY(g.get_nsb(x) ...)) so assertions reference the
actual node IDs instead of assumed insertion order.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
mixedprecision/pop/test_carry_analysis.cppmixedprecision/pop/test_expression_graph.cppmixedprecision/pop/test_pop_solver.cppmixedprecision/pop/test_simplex.cppmixedprecision/pop/test_ufp.cpp
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
2 participants
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.
Summary
-DUNIVERSAL_BUILD_MIXEDPRECISION_POP=ON)New tests by file
test_simplex.cpptest_expression_graph.cpptest_pop_solver.cpptest_ufp.cpptest_carry_analysis.cppTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests