more tests #532
more tests #532
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a large suite of new tests for Sugar→Salty conversion and rule normal-form checks, plus dozens of YAML condition-pack fixtures used by tests. No production source code changes except test files and test resource data; one export (isNF) is added to Rule for testing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds 18 new test cases to SugarSpec.hs to improve test coverage for the Sugar module's toSalty conversion functionality. The tests verify various edge cases and transformations between sweet (sugared) and salty (desugared) syntax representations in phi-calculus programs.
- Adds tests for expression conversions (EX_PHI_MEET, EX_PHI_AGAIN)
- Adds tests for various literal types (floats, negative numbers, empty strings, scientific notation)
- Adds tests for binding patterns, unicode attributes, and nested formations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EX_PHI_AGAIN pref ix innerSalty -> do | ||
| pref `shouldBe` Nothing | ||
| ix `shouldBe` 5 | ||
| innerSalty `shouldSatisfy` isDispatch |
There was a problem hiding this comment.
The variable name 'innerSalty' is a compound name. According to the project's coding guidelines, compound variable names make code harder to read, and single-word nouns are preferred for variable names.
| isDispatch EX_DISPATCH{} = True | ||
| isDispatch _ = False |
There was a problem hiding this comment.
The helper function 'isDispatch' is duplicated in this test. The same function definition appears in the test at lines 296-297. This code duplication reduces maintainability and should be extracted to avoid repetition.
| case salty of | ||
| EX_PHI_MEET pref ix innerSalty -> do | ||
| pref `shouldBe` Just "x" | ||
| ix `shouldBe` 1 | ||
| innerSalty `shouldSatisfy` isDispatch | ||
| _ -> fail "Expected EX_PHI_MEET but got different constructor" |
There was a problem hiding this comment.
This test contains multiple assertions (4 in total: 3 shouldBe assertions and 1 shouldSatisfy assertion). According to the project's testing guidelines, each test should contain only one assertion.
| case salty of | ||
| EX_PHI_AGAIN pref ix innerSalty -> do | ||
| pref `shouldBe` Nothing | ||
| ix `shouldBe` 5 | ||
| innerSalty `shouldSatisfy` isDispatch | ||
| _ -> fail "Expected EX_PHI_AGAIN but got different constructor" |
There was a problem hiding this comment.
This test contains multiple assertions (4 in total: 3 shouldBe assertions and 1 shouldSatisfy assertion). According to the project's testing guidelines, each test should contain only one assertion.
| expr' = EX_PHI_MEET (Just "x") 1 inner | ||
| salty = toSalty expr' | ||
| isDispatch EX_DISPATCH{} = True | ||
| isDispatch _ = False |
There was a problem hiding this comment.
This test contains a blank line (line 297) inside the test body. According to the project's coding guidelines, blank lines inside method bodies reduce code readability and should be removed.
| expr' = EX_PHI_AGAIN Nothing 5 inner | ||
| salty = toSalty expr' | ||
| isDispatch EX_DISPATCH{} = True | ||
| isDispatch _ = False |
There was a problem hiding this comment.
This test contains a blank line (line 311) inside the test body. According to the project's coding guidelines, blank lines inside method bodies reduce code readability and should be removed.
| EX_PHI_MEET pref ix innerSalty -> do | ||
| pref `shouldBe` Just "x" | ||
| ix `shouldBe` 1 | ||
| innerSalty `shouldSatisfy` isDispatch |
There was a problem hiding this comment.
The variable name 'innerSalty' is a compound name. According to the project's coding guidelines, compound variable names make code harder to read, and single-word nouns are preferred for variable names.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/SugarSpec.hs (1)
305-317: LGTM with minor observation.The test correctly verifies
EX_PHI_AGAINprocessing. TheisDispatchhelper is duplicated from the previous test (lines 296-297). For test code, inline helpers are acceptable for readability, though extracting to a top-level helper would reduce duplication if more tests need this predicate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/SugarSpec.hs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Agent
- GitHub Check: cabal (ubuntu-24.04)
- GitHub Check: cabal (macos-15)
- GitHub Check: cabal (windows-2022)
- GitHub Check: stack (windows-2022)
- GitHub Check: stack (macos-15)
- GitHub Check: stack (ubuntu-24.04)
🔇 Additional comments (9)
test/SugarSpec.hs (9)
291-303: LGTM!Well-structured test that properly verifies the recursive processing of
EX_PHI_MEETexpressions while preserving the wrapper structure. The pattern matching approach with explicit failure messaging is appropriate for this type of structural verification.
319-332: LGTM!Good coverage testing meta binding preservation both standalone (
BI_META) and within sequences (BDS_META).
334-341: LGTM!Good test verifying that existing rho bindings aren't duplicated when in first position with additional bindings.
343-349: LGTM!Good edge case test for single void parameter expansion, complementing the multi-parameter test at lines 218-224.
351-363: LGTM!Good additions testing numeric literal edge cases (floating point and negative numbers).
373-388: LGTM!Good edge case coverage: rho tau in non-first position and single positional argument conversion to alpha binding.
390-404: LGTM!Good coverage for multiple void parameters and unicode attribute handling. The unicode test is valuable for ensuring proper internationalization support.
406-418: LGTM!Good boundary case tests for empty string literals and scientific notation numbers.
420-436: LGTM!Excellent tests for deeply nested formations (validating recursive rho insertion at all levels) and the minimal case of formation with only a rho void binding.
| describe "toSalty handles single named application argument" $ | ||
| it "processes single tau binding correctly" $ do | ||
| prog <- parseProgramThrows "{Q.x(y -> $)}" | ||
| let cst = programToCST prog | ||
| salty = toSalty cst | ||
| rendered = render salty | ||
| rendered `shouldContain` "y" |
There was a problem hiding this comment.
Duplicate test detected.
This test is identical to the existing test at lines 125-130. Both parse "{Q.x(y -> $)}" and verify the output contains "y". Consider removing this duplicate or modifying it to test a different aspect.
- describe "toSalty handles single named application argument" $
- it "processes single tau binding correctly" $ do
- prog <- parseProgramThrows "{Q.x(y -> $)}"
- let cst = programToCST prog
- salty = toSalty cst
- rendered = render salty
- rendered `shouldContain` "y"📝 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.
| describe "toSalty handles single named application argument" $ | |
| it "processes single tau binding correctly" $ do | |
| prog <- parseProgramThrows "{Q.x(y -> $)}" | |
| let cst = programToCST prog | |
| salty = toSalty cst | |
| rendered = render salty | |
| rendered `shouldContain` "y" | |
| (lines before 365 remain unchanged) | |
| (lines 372 and onward continue as normal) |
🤖 Prompt for AI Agents
In test/SugarSpec.hs around lines 365-371 there is a duplicate test identical to
the one at lines 125-130: both parse "{Q.x(y -> $)}" and assert the rendered
output contains "y". Remove this duplicate test or replace it with a distinct
case (for example change the input to a different binding or assert a different
expected string) so the suite covers a new behavior rather than repeating the
existing assertion.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test-resources/condition-packs/nf-formation-with-void.yaml (1)
1-7: Well-formed fixture; quoting correctly avoids YAML?/!pitfalls.
- Lines 4-7: Using single-quoted scalars here is the right call because
?and!can be YAML syntax/tag tokens when unquoted; this should keep the fixture stable across YAML parsers.- Minor: consider adding a brief comment describing what “void inner formation” means in this fixture (helps future test maintenance).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
test-resources/condition-packs/and-fails.yaml(1 hunks)test-resources/condition-packs/and-multiple.yaml(1 hunks)test-resources/condition-packs/in-missing.yaml(1 hunks)test-resources/condition-packs/in-simple.yaml(1 hunks)test-resources/condition-packs/in-void.yaml(1 hunks)test-resources/condition-packs/nf-dispatch-global.yaml(1 hunks)test-resources/condition-packs/nf-dispatch-this.yaml(1 hunks)test-resources/condition-packs/nf-empty-formation.yaml(1 hunks)test-resources/condition-packs/nf-formation-with-delta.yaml(1 hunks)test-resources/condition-packs/nf-formation-with-void.yaml(1 hunks)test-resources/condition-packs/nf-global.yaml(1 hunks)test-resources/condition-packs/nf-termination.yaml(1 hunks)test-resources/condition-packs/nf-this.yaml(1 hunks)test-resources/condition-packs/not-alpha.yaml(1 hunks)test-resources/condition-packs/not-eq.yaml(1 hunks)test-resources/condition-packs/not-nf-on-dc.yaml(1 hunks)test-resources/condition-packs/or-first-match.yaml(1 hunks)test-resources/condition-packs/part-of-missing.yaml(1 hunks)test-resources/condition-packs/part-of-nested.yaml(1 hunks)test-resources/condition-packs/part-of-simple.yaml(1 hunks)test-resources/condition-packs/xi-application.yaml(1 hunks)test-resources/condition-packs/xi-dispatch.yaml(1 hunks)test-resources/condition-packs/xi-formation.yaml(1 hunks)test-resources/condition-packs/xi-global.yaml(1 hunks)test-resources/condition-packs/xi-this.yaml(1 hunks)test/RuleSpec.hs(3 hunks)
✅ Files skipped from review due to trivial changes (10)
- test-resources/condition-packs/in-void.yaml
- test-resources/condition-packs/nf-formation-with-delta.yaml
- test-resources/condition-packs/not-nf-on-dc.yaml
- test-resources/condition-packs/nf-termination.yaml
- test-resources/condition-packs/nf-global.yaml
- test-resources/condition-packs/in-missing.yaml
- test-resources/condition-packs/xi-this.yaml
- test-resources/condition-packs/in-simple.yaml
- test-resources/condition-packs/xi-formation.yaml
- test-resources/condition-packs/nf-this.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: stack (macos-15)
- GitHub Check: stack (ubuntu-24.04)
- GitHub Check: stack (windows-2022)
- GitHub Check: cabal (ubuntu-24.04)
- GitHub Check: cabal (macos-15)
- GitHub Check: cabal (windows-2022)
🔇 Additional comments (17)
test-resources/condition-packs/not-alpha.yaml (1)
4-8: Looks correct; please verify schema expectations fornot/alphaand the literal!a.
Quoting avoids YAML tag parsing issues (good), but ensure the condition-pack parser expectscondition.not.alphaas a string and thatalpha: '!a'(including the leading!) is the intended literal for this fixture.test-resources/condition-packs/nf-dispatch-global.yaml (1)
1-7: Looks correct as a minimal NF condition-pack fixture; please verify the intended “global dispatch” syntax (Q.foo).
YAML structure/indentation and quoting are solid (notably quoting!eavoids YAML tag parsing). Only thing to double-check is that the rule DSL indeed treatsQ.fooas the “global dispatch” case (and that the harness expects a single mapping, not a list of entries).test-resources/condition-packs/and-fails.yaml (1)
1-10: YAML fixture looks correct and unambiguous (quoting + AND shape).test-resources/condition-packs/not-eq.yaml (1)
1-8: Good NOT(EQ) fixture; quoting avoids YAML!tag pitfalls.test-resources/condition-packs/part-of-simple.yaml (1)
1-7: Looks like a solid “part-of” baseline fixture (simple + readable).test-resources/condition-packs/part-of-nested.yaml (1)
1-7: Nested “part-of” fixture is well-formed; quoting keeps it YAML-safe.test-resources/condition-packs/part-of-missing.yaml (1)
1-8: Negative “part-of” fixture (withfailure: true) is clean and consistent.test-resources/condition-packs/or-first-match.yaml (1)
1-9: Well-structured OR fixture; should exercise first-match behavior reliably.test-resources/condition-packs/and-multiple.yaml (1)
1-11: Compound AND + nested NOT(EQ) is formatted correctly and should be robust in parsers.test-resources/condition-packs/xi-dispatch.yaml (1)
1-7:xidispatch fixture is minimal and YAML-safe (quoted!e).test-resources/condition-packs/xi-global.yaml (1)
1-7: Good fixture; keep!equoted to avoid YAML tag parsing, and verify the loader reads it as a plain string.
The quotes around[[ ... ]]and!eare doing important work here; without them, YAML may interpret!eas a tag in some parsers. Consider adding/ensuring a test assertion that the parsed value ofcondition.xiequals the literal string"!e".test-resources/condition-packs/xi-application.yaml (1)
1-7: Good fixture; quoting is required here (both?and!e), and it’d be worth ensuring it’s actually exercised by a test.
Given YAML’s plain-scalar rules, keepingexpressionandxi: '!e'quoted avoids subtle parse surprises. If not already covered, add/ensure a test that loads this file and asserts the parsedexpression,pattern, andcondition.xistrings match exactly.test-resources/condition-packs/nf-dispatch-this.yaml (1)
1-7: LGTM! Well-structured test fixture.The YAML structure is valid and the test data appropriately captures a dispatch-on-this scenario for normal form checking. The expression, pattern, and condition are well-formed and align with the filename.
test-resources/condition-packs/nf-empty-formation.yaml (1)
1-7: LGTM! Appropriate empty formation test case.The test fixture correctly captures an empty formation scenario with valid YAML structure. The expression
[[ x -> [[ ]] ]]clearly tests the empty formation case for normal form validation.test/RuleSpec.hs (3)
32-59: LGTM! Robust condition checking test suite.The test suite correctly:
- Dynamically loads all condition pack fixtures from the resources directory
- Validates pattern matching produces non-empty results
- Distinguishes between expected failures and successes using the
failureflag- Provides helpful error messages with substitution details
This approach efficiently exercises the new condition pack fixtures added in this PR.
60-85: Excellent comprehensive normal form test coverage.The test suite thoroughly exercises
isNFacross diverse scenarios:
- Base cases (ExThis, ExGlobal, ExTermination)
- Dispatch operations on different expression types
- Various formation structures (empty, delta, void, lambda, mixed)
- Both positive and negative test cases
This comprehensive coverage significantly advances the PR objective of improving test coverage.
86-102: Comprehensive rule matching test coverage.The test suite appropriately covers three key scenarios for
matchProgramWithRule:
- Successful pattern match with condition met
- Pattern mismatch
- Pattern match with failed condition
Tests are well-structured and the required rule file exists.
|
@maxonfjvipon a few more tests |
test/RuleSpec.hs
Outdated
| isNF (ExFormation [BiLambda "Func"]) ctx `shouldBe` True | ||
| it "returns true for formation with delta void and lambda" $ | ||
| isNF (ExFormation [BiDelta (BtOne "FF"), BiVoid (AtLabel "y"), BiLambda "G"]) ctx `shouldBe` True | ||
| describe "matchProgramWithRule matches programs" $ do |
Co-authored-by: yegor256 <526301+yegor256@users.noreply.github.com>
Refactor isNF tests to use forM_ with tuples
Co-authored-by: yegor256 <526301+yegor256@users.noreply.github.com>
[WIP] Remove suggested test from previous pull request
|
@maxonfjvipon take a look again please |
|
@rultor merge |
@maxonfjvipon OK, I'll try to merge now. You can check the progress of the merge here. |
@maxonfjvipon Done! FYI, the full log is here (took me 11min). |
closes #516
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.