bug(#632): wrong-test-order lint#784
Conversation
WalkthroughAdds a new XSLT lint rule detecting unit tests placed before live objects, accompanying documentation and multiple YAML test cases (allowed and violating orders), and updates CI workflows to gate certain jobs via a RUN_WORKFLOW variable and adjust the Maven JDK matrix and exclusions. Changes
Sequence Diagram(s)sequenceDiagram
participant EO as EO Source (XML)
participant XSLT as XSLT Processor
participant Sheet as wrong-test-order.xsl
participant Defects as Defects (XML)
EO->>XSLT: parse source XML
XSLT->>Sheet: apply stylesheet
Sheet->>Sheet: find unit nodes with name starting "+"
Sheet->>Sheet: inspect following sibling
alt Violation (test before live object)
Sheet->>Defects: emit <defect line="..." severity="warning">The unit test wrongly ordered: ...</defect>
else No violation
Note right of Sheet: no defect emitted
end
Defects-->>XSLT: return defects document
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
🔇 Additional comments (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 |
![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 (2)
src/test/resources/org/eolang/lints/packs/single/wrong-test-order/allows-correct-order.yaml (1)
1-13: Test logic is correct, but the filename is semantically misleading.The test correctly detects a violation (test on line 3 followed by live object on line 4), but the filename
allows-correct-order.yamlsuggests it's validating correct ordering. Consider renaming tocatches-live-after-test.yamlor similar to better reflect the violation scenario being tested.src/test/resources/org/eolang/lints/packs/single/wrong-test-order/catches-wrong-order.yaml (1)
1-12: Test logic is correct, but the filename is semantically misleading.The test correctly expects no violations when the code contains only live objects (no
+>test units). However, the filenamecatches-wrong-order.yamlsuggests the test detects a violation, when it actually validates the absence of violations. Consider renaming toallows-no-tests.yamlorvalidates-live-objects-only.yamlfor clarity. (Note:allows-no-tests.yamlalready exists; consider naming based on the distinction if intentional.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/resources/org/eolang/lints/tests/wrong-test-order.xsl(1 hunks)src/main/resources/org/eolang/motives/tests/wrong-test-order.md(1 hunks)src/test/resources/org/eolang/lints/packs/single/wrong-test-order/allows-correct-order.yaml(1 hunks)src/test/resources/org/eolang/lints/packs/single/wrong-test-order/allows-no-tests.yaml(1 hunks)src/test/resources/org/eolang/lints/packs/single/wrong-test-order/allows-single-test.yaml(1 hunks)src/test/resources/org/eolang/lints/packs/single/wrong-test-order/allows-tests-only.yaml(1 hunks)src/test/resources/org/eolang/lints/packs/single/wrong-test-order/catches-mixed.yaml(1 hunks)src/test/resources/org/eolang/lints/packs/single/wrong-test-order/catches-test-first.yaml(1 hunks)src/test/resources/org/eolang/lints/packs/single/wrong-test-order/catches-wrong-order.yaml(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). (10)
- GitHub Check: ort
- GitHub Check: mvn (macos-15, 21)
- GitHub Check: mvn (ubuntu-24.04, 21)
- GitHub Check: qulice
- GitHub Check: mvn (windows-2022, 21)
- GitHub Check: mvn (macos-15, 11)
- GitHub Check: mvn (windows-2022, 11)
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: deep
- GitHub Check: reserved
🔇 Additional comments (12)
src/main/resources/org/eolang/motives/tests/wrong-test-order.md (1)
1-19: Documentation is clear and well-structured.The examples effectively illustrate the rule: unit tests (marked with
+>) must follow live objects (marked with>). The structure and formatting align well with the test cases that validate this lint.src/test/resources/org/eolang/lints/packs/single/wrong-test-order/catches-test-first.yaml (1)
1-13: Test structure and assertions are correct.The test properly validates detection of a test unit appearing before a non-test unit. The assertions and input accurately reflect the violation being tested.
src/test/resources/org/eolang/lints/packs/single/wrong-test-order/allows-no-tests.yaml (1)
1-12: Test correctly validates no violations when tests are absent.This edge case ensures the lint properly handles code containing only live objects without false positives.
src/test/resources/org/eolang/lints/packs/single/wrong-test-order/catches-mixed.yaml (1)
1-13: Test correctly handles mixed test/non-test ordering.The scenario with interleaved tests and non-tests is properly validated, flagging the first violation at line 2.
src/test/resources/org/eolang/lints/packs/single/wrong-test-order/allows-single-test.yaml (1)
1-10: Test validates the edge case of a single test with no following sibling.Properly confirms that the lint accepts a test unit when it has no subsequent siblings to create an ordering violation.
src/test/resources/org/eolang/lints/packs/single/wrong-test-order/allows-tests-only.yaml (1)
1-11: Test validates correct consecutive test ordering.The scenario with multiple tests grouped together and following live objects is properly validated, confirming no violations are flagged for this correct pattern.
src/main/resources/org/eolang/lints/tests/wrong-test-order.xsl (6)
1-5: LGTM!Standard XML declaration and copyright header are properly formatted.
11-11: LGTM!The output declaration is correctly configured for XML output.
12-14: LGTM!The template structure and XPath selection logic correctly identify test objects (those with names starting with '+'). The use of
//owill traverse all depths, which is appropriate for this lint rule.
15-16: LGTM!The logic correctly identifies ordering violations by checking if a test object (name starting with '+') is followed by a non-test object. The use of
following-sibling::*[1]ensures only the immediate next sibling is checked, which is the right approach for detecting ordering issues.
17-29: LGTM!The defect element construction is well-structured with appropriate attributes (line number, optional context for line 0, and warning severity).
30-31: Unable to locate theeo:escape-plusfunction definition to verify the double escaping intent.The function
eo:escape-plusis imported from/org/eolang/parser/_funcs.xsl, which does not exist in this repository's source. This suggests the function is provided by an external dependency. Theeo:escapefunction itself performs XML escaping (quotes and spaces).The double escaping pattern
eo:escape(eo:escape-plus(@name))is used consistently in two locations (wrong-test-order.xsl:31 and unit-test-without-phi.xsl:29), and escape-plus is also used independently in incorrect-test-object-name.xsl:16, which suggests the behavior is intentional. However, without access to the escape-plus definition, the exact sequence of transformations cannot be verified.Optional grammar improvement: Line 30 message "The unit test wrongly ordered:" could be more concise as "Unit test is wrongly ordered:" or "Unit test is in wrong order."
github-actions
bot
added
the
core
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/hone.yml(2 hunks).github/workflows/jmh.yml(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: actionlint
.github/workflows/jmh.yml
[error] 16-16: context "env" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
.github/workflows/hone.yml
[error] 24-24: context "env" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
⏰ 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). (8)
- GitHub Check: reserved
- GitHub Check: deep
- GitHub Check: mvn (windows-2022, 11)
- GitHub Check: mvn (macos-15, 11)
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: mvn (windows-2022, 21)
- GitHub Check: mvn (ubuntu-24.04, 21)
- GitHub Check: ort
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/mvn.yml (1)
20-24: Clarify platform-specific Java 17 exclusions and add inline documentation.The exclusion of Java 17 from windows-2022 and macos-15 is unusual, as Java is officially cross-platform. This suggests either:
- Known compatibility issues with these runners that warrant exclusion, or
- A temporary workaround that should be revisited.
Without inline documentation, future maintainers may not understand the rationale. Consider adding a comment explaining why Java 17 is excluded on these platforms (e.g., hardware limitations, toolchain incompatibilities, known JVM bugs).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/mvn.yml(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). (6)
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: ort
- GitHub Check: reserved
- GitHub Check: deep
🔇 Additional comments (2)
.github/workflows/mvn.yml (2)
19-19: Verify Java version upgrade rationale.The Java matrix was changed from [11, 21] to [17, 23], completely removing Java 21 (the current LTS version). Java 23 is a non-LTS release (released Sept 2024), which is unusual as a primary CI test target. Before merging, confirm:
- Is Java 21 (LTS) intentionally no longer supported?
- Why test Java 23 (non-LTS) instead of the current LTS (21)?
- Does this align with the project's Java support policy?
1-38: PR scope clarification: Lint rule implementation not visible in this file.The PR objectives mention implementing a new
wrong-test-orderlint rule, but this file contains only CI workflow configuration updates (Java version and platform exclusions). If the lint rule implementation is in other files, ensure those files are also included in the review scope. If this is the only change, verify alignment between the PR title and actual changes.
|
@maxonfjvipon take a look, please |
|
@yegor256 take a look, please |
3 participants
In this pull I've implemented new lint:
wrong-test-order, that issues defect withwarningseverity if test attributes are wrongly ordered.see #632
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.