Added lint for fix of comments in test code duplicate test names #752
Conversation
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
WalkthroughAdds a package-private lint Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant LintRunner
participant LtTestComment
participant XMIR as "XMIR Document"
LintRunner->>LtTestComment: defects(XML)
LtTestComment->>XMIR: select //o[@name and starts-with(@name,'+')]
XMIR-->>LtTestComment: matching <o> nodes
loop each +test node
LtTestComment->>XMIR: search .//meta[@key='comment']
alt comment present
LtTestComment-->>LintRunner: report defect id="test-has-comment" severity=WARNING line=@line
else
LtTestComment-->>LintRunner: no defect
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/test/java/org/eolang/lints/LtTestCommentTest.java (1)
3-3: Remove unused import.
Xnavisn’t used in this test class.-import com.github.lombrozo.xnav.Xnav;src/main/java/org/eolang/lints/LtTestComment.java (1)
23-23: Avoid zero-capacity allocation.Use default constructor; capacity hint of 0 is misleading.
- final Collection<Defect> defects = new ArrayList<>(0); + final Collection<Defect> defects = new ArrayList<>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/eolang/lints/LtTestComment.java(1 hunks)src/test/java/org/eolang/lints/LtTestCommentTest.java(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: copyrights
src/test/java/org/eolang/lints/LtTestCommentTest.java
[warning] 1-1: Missed copyright header.
src/main/java/org/eolang/lints/LtTestComment.java
[warning] 1-1: Missed copyright header.
🪛 GitHub Actions: qulice
src/test/java/org/eolang/lints/LtTestCommentTest.java
[error] 1-1: Test name 'motiveReturnsExpectedString' doesn't follow naming rules, because the test name has to be written using present tense.
[error] 1-1: Method 'motiveReturnsExpectedString' has assertion without message: 'MatcherAssert.assertThat(lint.motive(), Matchers.containsString("Comments in test objects are discouraged"))'. Please add the explanation message to make the test more readable.
[error] 1-1: Test name 'nameReturnsExpectedValue' doesn't follow naming rules, because the test name has to be written using present tense.
[error] 1-1: Method 'nameReturnsExpectedValue' has assertion without message: 'MatcherAssert.assertThat(lint.name(), Matchers.equalTo("test-has-comment"))'. Please add the explanation message to make the test more readable.
[error] 1-1: Test name 'returnsNoDefectWhenTestObjectHasNoComment' doesn't follow naming rules, because the test name has to be written using present tense.
[error] 1-1: Method 'returnsNoDefectWhenTestObjectHasNoComment' has assertion without message: 'MatcherAssert.assertThat(defects, Matchers.empty())'. Please add the explanation message to make the test more readable.
[error] 1-1: Test name 'returnsNoDefectWhenNoTestObjectPresent' doesn't follow naming rules, because test name doesn't have to contain the word 'test'.
[error] 1-1: Method 'returnsNoDefectWhenNoTestObjectPresent' has assertion without message: 'MatcherAssert.assertThat(defects, Matchers.empty())'. Please add the explanation message to make the test more readable.
⏰ 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: ort
- GitHub Check: mvn (windows-2022, 11)
- GitHub Check: mvn (ubuntu-24.04, 21)
- GitHub Check: mvn (macos-15, 11)
- GitHub Check: mvn (windows-2022, 21)
- GitHub Check: mvn (macos-15, 21)
- GitHub Check: deep
- GitHub Check: mvn (ubuntu-24.04, 11)
🔇 Additional comments (1)
src/main/java/org/eolang/lints/LtTestComment.java (1)
1-4: Align license header with repo expectations.CI warns about a missing/mismatched header. Ensure this header exactly matches what Qulice expects in this repository (copy from an existing passing file).
Would you like me to scan the repo for the canonical header and propose an exact replacement?
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/eolang/lints/LtTestComment.java (2)
25-33: Single-pass XPath to avoid extra allocationsYou can filter objects with comments directly in XPath and drop the per-object
commentstraversal.- final List<Xnav> objects = xml - .path("//o[@name and starts-with(@name, '+')]") - .collect(Collectors.toList()); - for (final Xnav object : objects) { - final List<Xnav> comments = object - .path(".//meta[@key='comment']") - .collect(Collectors.toList()); - if (!comments.isEmpty()) { - defects.add( + final List<Xnav> objects = xml + .path("//o[@name and starts-with(@name, '+')][.//meta[@key='comment']]") + .collect(Collectors.toList()); + for (final Xnav object : objects) { + defects.add( new Defect.Default( "test-has-comment", Severity.WARNING, - new OnDefault(xmir).get(), - Integer.parseInt(object.attribute("line").text().orElse("0")), + new OnDefault(xmir).get(), + Integer.parseInt(object.attribute("line").text().orElse("0")), "Test object contains a comment. Prefer short, self-explanatory test names instead of documenting them." ) ); - } - } + }
24-24: Avoid re-instantiatingOnDefaultinside the loopMinor allocation nit: create it once and reuse
origin.get().- final Xnav xml = new Xnav(xmir.inner()); + final Xnav xml = new Xnav(xmir.inner()); + final OnDefault origin = new OnDefault(xmir); ... - new OnDefault(xmir).get(), + origin.get(),Also applies to: 37-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/eolang/lints/LtTestComment.java(1 hunks)src/test/java/org/eolang/lints/LtTestCommentTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/eolang/lints/LtTestCommentTest.java
⏰ 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: mvn (ubuntu-24.04, 21)
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: mvn (windows-2022, 21)
- GitHub Check: mvn (macos-15, 21)
- GitHub Check: mvn (macos-15, 11)
- GitHub Check: mvn (windows-2022, 11)
- GitHub Check: deep
- GitHub Check: ort
🔇 Additional comments (3)
src/main/java/org/eolang/lints/LtTestComment.java (3)
25-27: Root-agnostic XPath fix LGTMUsing
//o[@name and starts-with(@name, '+')]correctly finds test objects regardless of the document root.
29-31: Descendant search for comment meta is correct
.path(".//meta[@key='comment']")will catch comments not only as direct children; aligns with the intent of the lint.
29-32: Confirm scope: should comments in nested helper objects trigger this lint?Current descendant search flags any
meta[@key='comment']under a test object, including nested helper objects’ comments. If the rule intends to only forbid comments attached to the test object itself, restrict to direct child metas. Otherwise keep as-is. Consider adding a unit test with a nested object comment to lock the intended behavior.
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
|
@kislayykumar Thanks for your contribution, we appreciate it! Before we can merge this pull request, we need to see all CI workflows pass without errors. We can't merge the pull request into the master branch unless all workflows are green. Try to fix them, also paying attention to code style issues. |
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/org/eolang/lints/LtByXslTest.java (1)
362-362: 7-minute timeout is excessive; prefer expressing as minutes and revisit root causeBumping to 420s can hide regressions. Since this test is tagged "deep", consider keeping a tighter budget or splitting work; if long runtime is unavoidable, make the timeout self-explanatory.
Apply for readability:
- @Timeout(420L) + @Timeout(value = 7, unit = TimeUnit.MINUTES)Add import:
import java.util.concurrent.TimeUnit;Please confirm typical runtime locally and on CI, and whether "deep" tests are excluded from regular PR runs (kept for nightly).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/org/eolang/lints/LtByXslTest.java(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). (8)
- GitHub Check: mvn (macos-15, 11)
- GitHub Check: deep
- GitHub Check: mvn (windows-2022, 21)
- GitHub Check: mvn (macos-15, 21)
- GitHub Check: mvn (ubuntu-24.04, 21)
- GitHub Check: mvn (windows-2022, 11)
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: ort
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
Signed-off-by: KISLAY KUMAR <135631873+kislayykumar@users.noreply.github.com>
|
Hi @yegor256 Kindly review the PR and merge it. All workflows and checks have passed. Thank you! |
|
@h1alexbel please, review this one |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/test/java/org/eolang/lints/LtTestCommentTest.java (1)
1-4: Header fixed — matches repo SPDX style.This resolves the copyrights check.
🧹 Nitpick comments (4)
src/test/java/org/eolang/lints/LtTestCommentTest.java (4)
22-34: Drop unnecessary throws in test signature.No IO occurs; remove
throws IOExceptionfor cleanliness.- void reportsNoDefectsWhenNoCommentPresent() throws IOException { + void reportsNoDefectsWhenNoCommentPresent() {
36-48: Same: remove redundant throws.- void reportsNoDefectsWhenNoObjectPresent() throws IOException { + void reportsNoDefectsWhenNoObjectPresent() {
50-58: And here: no IO — remove throws.- void containsGuidanceInMotive() throws IOException { + void containsGuidanceInMotive() {Also remove the now‑unused import:
-import java.io.IOException;
68-70: Add a positive test (comment present → defect reported).Covers the core path and prevents regressions.
} + + @Test + void reportsDefectWhenTestObjectHasComment() { + final XML xml = new XMLDocument( + "<program>" + + "<object name='foo' line='10'>" + + "<o name='+test' line='12'>" + + "<meta key='comment' line='13'/>" + + "</o>" + + "</object>" + + "</program>" + ); + final LtTestComment lint = new LtTestComment(); + final Collection<Defect> defects = lint.defects(xml); + MatcherAssert.assertThat( + "Should report exactly one defect when a test object has a comment", + defects, + Matchers.hasSize(1) + ); + } +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/org/eolang/lints/LtByXslTest.java(2 hunks)src/test/java/org/eolang/lints/LtTestCommentTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/eolang/lints/LtByXslTest.java
🔇 Additional comments (1)
src/test/java/org/eolang/lints/LtTestCommentTest.java (1)
60-68: LGTM: stable id and assertion message.
h1alexbel
left a comment
h1alexbel
left a comment
There was a problem hiding this comment.
@kislayykumar take a look, please
| * Lint that warns if a comment is present at a test object. | ||
| * @since 0.0.59 | ||
| */ | ||
| final class LtTestComment implements Lint<XML> { |
There was a problem hiding this comment.
@kislayykumar looks like we can have this lint in XSL, there are no restrictions to implement it in Java, I believe. WDYT?
| public Collection<Defect> defects(final XML xmir) throws IOException { | ||
| final Collection<Defect> defects = new ArrayList<>(0); | ||
| final Xnav xml = new Xnav(xmir.inner()); | ||
| final List<Xnav> objects = xml |
There was a problem hiding this comment.
@kislayykumar this variable seems to be redundant, we can inlline it in the forEach loop
| .path("//o[@name and starts-with(@name, '+')]") | ||
| .collect(Collectors.toList()); | ||
| for (final Xnav object : objects) { | ||
| final List<Xnav> comments = object |
There was a problem hiding this comment.
@kislayykumar redundant variable, we should inline it in the if. Also, read this, please: https://www.yegor256.com/2015/09/01/redundant-variables-are-evil.html
|
|
||
| @Override | ||
| public String motive() throws IOException { | ||
| return "Avoid comments in test objects; use clear test names."; |
There was a problem hiding this comment.
@kislayykumar let's write more descriptive motive for lint. Please check src/main/resources/org/eolang/motives as reference for convention
| final XML xml = new XMLDocument( | ||
| "<program><object name='foo' line='10'><o name='+test' line='12'></o></object></program>" | ||
| ); | ||
| final LtTestComment lint = new LtTestComment(); |
|
|
||
| @Test | ||
| void containsGuidanceInMotive() throws IOException { | ||
| final LtTestComment lint = new LtTestComment(); |
|
|
||
| @Test | ||
| void returnsStableId() { | ||
| final LtTestComment lint = new LtTestComment(); |
|
|
||
| @Test | ||
| void reportsNoDefectsWhenNoCommentPresent() throws IOException { | ||
| final XML xml = new XMLDocument( |
There was a problem hiding this comment.
@kislayykumar let's use org.eolang.parser.EoSyntax for constructing EO program
| * | ||
| * @since 0.0.1 | ||
| */ | ||
| final class LtTestCommentTest { |
There was a problem hiding this comment.
@kislayykumar as @coderabbitai outlined, let's introduce test that would check how lint works for tests that have comments above:
[] > foo
# This test checks how my object works:
[] +> calculates-critical-path
There was a problem hiding this comment.
@h1alexbel I'll add a test case that verifies the lint correctly detects comments in test objects. Let me generate the test method that uses your EO example:
@Test
void detectsCommentInTestObject() throws IOException {
final XML xml = new XMLDocument(
String.join(
"",
"<?xml version='1.0' encoding='UTF-8'?>",
"<program>",
"<objects>",
"<o abstract='true' line='1' name='foo'>",
"<o line='3' name='+calculates-critical-path'>",
"<meta key='comment' value='This test checks how my object works:'/>",
"</o>",
"</o>",
"</objects>",
"</program>"
)
);
final Collection<Defect> defects = new LtTestComment().defects(xml);
MatcherAssert.assertThat(
"Should detect comment in test object",
defects,
Matchers.hasSize(1)
);
final Defect defect = defects.iterator().next();
MatcherAssert.assertThat(
"Defect should have correct ID",
defect.name(),
Matchers.equalTo("test-has-comment")
);
MatcherAssert.assertThat(
"Defect should have correct message",
defect.message(),
Matchers.equalTo("Test object contains a comment. Prefer self-explanatory test names.")
);
}This test case:
- Creates XML representing your EO code example with a test object
+calculates-critical-paththat has a comment - Verifies that
LtTestComment.defects()finds exactly one defect - Checks that the defect has the correct ID and message as expected from the lint implementation
The XML structure follows the XMIR format where the comment from EO code gets converted to a <meta key='comment' value='...'/> element, which is what the LtTestComment lint searches for.
| .collect(Collectors.toList()); | ||
| for (final Xnav object : objects) { | ||
| final List<Xnav> comments = object | ||
| .path(".//meta[@key='comment']") |
There was a problem hiding this comment.
@kislayykumar In EO we have this:
[] > foo
# Comment is here.
[] +> my-test
After parsing, we have the following XMIR:
<object >
<comments>
<comment line="3">Comment is here</comment>
</comments>
<o line="1" name="foo" pos="0">
<o base="ξ" line="1" name="xi🌵" pos="0"/>
<o line="3" name="+my-test" pos="2">
<o base="ξ" line="3" name="xi🌵" pos="2"/>
</o>
</o>
</object>3 participants
Fix for #748 (comment)
Summary by CodeRabbit
New Features
Tests