test: Cover IfStmt float condition and VariableDeclaration zero-init paths#236
Conversation
|
@yuvimittal , PR ready for a review! |
|
Hi @yuvimittal |
|
@Jaskirat-s7 , generally when we make a branch, we name it starting |
| (astx.Int16, astx.LiteralInt16), | ||
| (astx.Int8, astx.LiteralInt8), | ||
| (astx.Int64, astx.LiteralInt64), | ||
| (astx.Float32, astx.LiteralFloat32), |
There was a problem hiding this comment.
Done! Added (astx.Float64, astx.LiteralFloat64) to both test_if_else_stmt and test_if_only_stmt parametrize lists.
|
Hey @yuvimittal — quick question on workflow before I start Phase 4. Phase 3 PR (#239) is currently stacked on top of Phase 2 (#236). This works for now, but as I continue toward 90% coverage, I'm wondering what's preferred: Option A — Keep stacking: Branch Phase 4 from Phase 3, Phase 5 from Phase 4, etc. Downside is that any change to Phase 2 forces a rebase cascade through all later phases. Option B — Branch independently from main: Open a sub-issue for Phase 4, branch directly from main, and keep each PR self-contained. This means Phase 4 may have slight duplication in scope with the unmerged Phase 2/3 commits, but would be much easier to review and merge independently. |
tests/test_variable_assignment.py
Outdated
|
|
||
| module.block.append(fn_main) | ||
|
|
||
| check_result("build", builder, module) |
There was a problem hiding this comment.
For this test to be useful, it should assert "0" for Int32 and "0.0" (or "0.000000", depending on how your IR formats it) for Float32. Without that, the test only checks that the build doesn't crash, not that it defaults to zero.
There was a problem hiding this comment.
i have added PrintExpr to print x and asserted "0" for Int32 and "0.000000" for Float32/Float64. The test now proves the default-to-zero behavior at runtime, not just that the build doesn't crash , also i added Float64 to the parametrize list.
tests/test_variable_assignment.py
Outdated
| def test_variable_declaration_no_initializer( | ||
| builder_class: type[Builder], | ||
| var_type: type, | ||
| literal_type: type, |
There was a problem hiding this comment.
why have you added this? this seems unused?
There was a problem hiding this comment.
You're right , literal_type was left over from the original structure and wasn't used anywhere in the function body. Removed it from the parametrize tuple, function signature, and docstring. All 3 cases still pass.
tests/test_variable_assignment.py
Outdated
| "var_type, literal_type", | ||
| [ | ||
| (astx.Int32, astx.LiteralInt32), | ||
| (astx.Float32, astx.LiteralFloat32), |
There was a problem hiding this comment.
@pytest.mark.parametrize( "var_type, expected_output", [ (astx.Int8, "0"), (astx.Int16, "0"), (astx.Int32, "0"), (astx.Int64, "0"), (astx.Float32, "0.0"), (astx.Float64, "0.0"), ], )
If zero-initialization is a general contract of your IR, you should cover more types for consistency
There was a problem hiding this comment.
i have added Int8, Int16, and Int64 alongside the existing Int32, Float32, Float64 , all 6 types now assert "0" (integers) or "0.000000" (floats) at runtime. All pass.
tests/test_variable_assignment.py
Outdated
There was a problem hiding this comment.
add float datatype here as well
There was a problem hiding this comment.
it was already included in the previous commit , Float32 and Float64 are in the parametrize list with "42.000000" as the expected output.
1537186 to
58792ba
Compare
|
@yuvimittal , i have added PrintExpr + expected_output assertion ('0' for ints, '0.000000' for floats) to prove zero-default at runtime, not just build success also removed unused literal_type parameter from signature and parametrize and added Int8, Int16, Int64 alongside Int32/Float32/Float64 for consistency |
…n zero-init paths - Add Float32 to test_if_else_stmt and test_if_only_stmt parametrize lists - Add test_variable_declaration_no_initializer for Int32 and Float32 - Fix: VariableDeclaration visitor checked 'node.value is not None' but astx defaults value to astx.Undefined(); added isinstance check so uninitialized vars reach the zero-init path (llvmliteir.py lines 3023-3059)
…tion_no_initializer
Add PrintExpr and expected_output assertion ('0' for Int32,
'0.000000' for Float32/Float64) so the test proves default-to-zero
behavior at runtime, not just that the build does not crash.
Also add Float64 to the parametrize list.
… review
- add PrintExpr + expected_output assertion ('0' for ints, '0.000000'
for floats) to prove zero-default at runtime, not just build success
- remove unused literal_type parameter from signature and parametrize
- add Int8, Int16, Int64 alongside Int32/Float32/Float64 for consistency
9b267fa to
38fb079
Compare
|
thanks @Jaskirat-s7 |
|
🎉 This PR is included in version 1.9.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary
Phase 2 coverage improvements for issue #38.
Changes
1. tests/test_if_stmt.py
Added Float32 to the int_type parametrize in test_if_else_stmt and test_if_only_stmt, covering the fcmp_ordered float condition path in the IfStmt visitor (llvmliteir.py lines 1191–1193).
2. tests/test_variable_assignment.py
Added test_variable_declaration_no_initializer — parametrized for Int32 and Float32 — covering the zero-init fallback in the VariableDeclaration visitor (llvmliteir.py lines 3049–3059).
3. Bug fix in llvmliteir.py
VariableDeclaration checked
node.value is not None, but astx defaults value to astx.Undefined() (not None), making the zero-init path unreachable. Fixed by also checking isinstance(node.value, astx.Undefined).Coverage Impact
Closes part of #38