-
Notifications
You must be signed in to change notification settings - Fork 292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Programming exercises
: Concatenate testsuite names with testcase names in LocalCI
#9691
base: develop
Are you sure you want to change the base?
Programming exercises
: Concatenate testsuite names with testcase names in LocalCI
#9691
Conversation
Programming exercises
: Concatenate testsuite names with testcase namesProgramming exercises
: Concatenate testsuite names with testcase names in LocalCI
WalkthroughThe pull request introduces significant modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/test/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParserTest.java (3)
262-292
: Consider enhancing test data with failure cases.The test effectively validates the handling of multiple top-level test suites. However, it could be more comprehensive by including failure cases to ensure proper name concatenation in error scenarios.
Consider adding a failing test case to verify error message formatting:
<testsuite name="SuiteB" tests="3"> <testcase name="Test1"/> <testcase name="Test2"/> - <testcase name="Test3"/> + <testcase name="Test3"> + <failure message="Test failed">Error details</failure> + </testcase> </testsuite>
294-314
: Consider adding assertions for empty suite names.The test effectively validates the handling of missing suite names. However, it could be more explicit in verifying that empty suite names are properly handled.
Consider adding test cases with empty names and mixed scenarios:
<testsuite> - <testsuite> + <testsuite name=""> <testsuite> <testcase name="Test1"/> <testcase name="Test2"/> <testcase name="Test3"/> </testsuite> </testsuite> </testsuite>Also consider adding assertions to explicitly verify that empty suite names are handled correctly:
assertThat(successfulTests).extracting(BuildResult.LocalCITestJobDTO::getName).containsExactlyInAnyOrder("Test1", "Test2", "Test3"); +// Verify that empty suite names don't affect the test names +assertThat(successfulTests).extracting(BuildResult.LocalCITestJobDTO::getName).allMatch(name -> !name.contains(".")); assertThat(failedTests).isEmpty();
243-314
: Comprehensive test coverage for concatenated test names.The test class effectively covers the requirements from issue #9688 with:
- Nested test suites with multiple levels
- Multiple top-level test suites
- Edge cases with missing suite names
- Realistic test data from Haskell tests
The test methods are well-structured and follow consistent patterns, making the test suite maintainable and easy to understand.
Consider extracting common test XML strings into constants or test resources if they might be reused by other test classes in the future.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParser.java (1)
25-27
: Enhance clarity in Javadoc commentsConsider rephrasing the comments for better clarity and grammatical correctness. For example:
- "The names of nested
testsuite
elements are prepended to thetestcase
name with dots."- "A singular top-level
testsuite
name is not included in the test case names."- "If multiple top-level
testsuite
elements are present, their names will be included."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParser.java
(2 hunks)src/test/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParserTest.java
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParser.java (1)
Pattern src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParserTest.java (1)
Pattern src/test/java/**/*.java
: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
📓 Learnings (2)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParser.java (3)
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8677
File: src/main/java/de/tum/in/www1/artemis/service/connectors/localci/buildagent/TestResultXmlParser.java:0-0
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The `TestSuite` constructor in `TestResultXmlParser.java` was improved by using `Objects.requireNonNullElse` to handle null values concisely.
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8677
File: src/main/java/de/tum/in/www1/artemis/service/connectors/localci/buildagent/TestResultXmlParser.java:0-0
Timestamp: 2024-10-08T15:35:48.768Z
Learning: The `TestSuite` constructor in `TestResultXmlParser.java` was improved by using `Objects.requireNonNullElse` to handle null values concisely.
Learnt from: magaupp
PR: ls1intum/Artemis#9490
File: src/main/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParser.java:54-56
Timestamp: 2024-10-15T11:00:39.014Z
Learning: In the Artemis codebase, test report XML files are manually written and are unlikely to have deeply nested `<testsuite>` elements. Therefore, using recursion in `TestResultXmlParser.processTestSuite` is acceptable, and the risk of stack overflow is minimal.
src/test/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParserTest.java (3)
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8677
File: src/main/java/de/tum/in/www1/artemis/service/connectors/localci/buildagent/TestResultXmlParser.java:0-0
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The `TestSuite` constructor in `TestResultXmlParser.java` was improved by using `Objects.requireNonNullElse` to handle null values concisely.
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8677
File: src/main/java/de/tum/in/www1/artemis/service/connectors/localci/buildagent/TestResultXmlParser.java:0-0
Timestamp: 2024-10-08T15:35:48.768Z
Learning: The `TestSuite` constructor in `TestResultXmlParser.java` was improved by using `Objects.requireNonNullElse` to handle null values concisely.
Learnt from: magaupp
PR: ls1intum/Artemis#9490
File: src/main/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParser.java:54-56
Timestamp: 2024-10-15T11:00:39.014Z
Learning: In the Artemis codebase, test report XML files are manually written and are unlikely to have deeply nested `<testsuite>` elements. Therefore, using recursion in `TestResultXmlParser.processTestSuite` is acceptable, and the risk of stack overflow is minimal.
🔇 Additional comments (4)
src/test/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParserTest.java (1)
243-260
: LGTM! Well-structured test for nested test suites.
The test case effectively validates the concatenation of test suite names, matching the requirements from issue #9688. The assertions properly verify that the full hierarchy of test suite names is preserved in the test case names.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParser.java (3)
56-58
: Implementation of processTopLevelTestSuite
looks good
The processTopLevelTestSuite
method correctly delegates to processTestSuiteWithNamePrefix
with an empty prefix, effectively handling single top-level test suites.
60-70
: Efficient handling of nested test suites
The processInnerTestSuite
method appropriately constructs the namePrefix
and recursively processes nested test suites, aligning with the PR objectives to include nested testsuite
names in test case names.
94-95
: Appropriate inclusion of name
attribute in TestSuite
Adding the name
attribute to the TestSuite
record enhances the ability to construct prefixed test case names, and utilizing Objects.requireNonNullElse
for initializing lists ensures null safety.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParser.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParser.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick fix. Code looks good.
I’ll test tomorrow and also adapt the Jenkins plugin to handle the <testsuites>
case.
Edit: Apparently I already implemented the <testsuites>
case a while ago (ls1intum/jenkins-server-notification-plugin#29). Just need to check that the naming is consistent with the way Artemis handles it now, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just recently introduced that handling of nested test suites into Artemis, right? So there should not be a lot of exercises on production instanced that would be affected by this change, right? Since the test case name is used as an identifier, a change in how they are read could break a lot of exercises, so I want to make sure that this only affects littes (to basically none) currently active exercises. |
@Strohgelaender PR #9490 introduced this recently. So only Haskell exercises created with or after Artemis version 7.6.1 might be affected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// A toplevel <testsuites> element is parsed like a <testsuite> | ||
processTestSuite(testSuite, failedTests, successfulTests); | ||
// The toplevel element can be <testsuites> or <testsuite> | ||
if (testResultFileString.contains("<testsuites")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to to the string for this check? Can't this be done on the parsed objects?
if (testResultFileString.contains("<testsuites")) { | |
if (testSuite.testSuites().size() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not that easy, I'm afraid. Both <testsuites>
and <testsuite>
can contain other testsuites. We need to distinct a <testsuites>
root element from a <testsuite>
root element because the name of the highest-level <testsuite>
tag needs to be ignored. This means the name of a <testsuite>
under the root <testsuites>
is ignored, while the name of a <testsuite>
under a root <testsuite>
is kept because the root's name is ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked for a way to check for the root tag with Jackson but wasn't able to find one. Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you have a boolean-parameter like testsuitesFound
that gets passed through recursivly? This can be used to decided if the name should be appended, and the value gets changed one the level got round.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean. The impact of the <testsuites>
element is only on its direct child <testsuite>
elements. All deeper nested <testsuite>
s should have their names prepended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure if my understanding of what you want to accive is correct now (Btw, if this is a quite complicated logic we should test all these cases and further extend the current server tests)
From my interpretation the main difference if wheter when traversing the nested testsuite
and testsuites
we have hit a point where we found a testsuites
element. Then, the name construction below should only use the local name. So we can just pass a boolean-Parameter that tells the level below if the top-level is such a element, and then create the name accordingly.
Well, since you wrote hightest level
we would need anotother boolean to indicate if we already found one such element before. With that, we should have all information we neeed (what is me parent, and was that parent the first such element) to create a fitting name.
Again, this behaviour should be documented and tested. I can already see it breaking just becuase the next developer didi not even know about testsuites in testsuites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't really a JUnit XML standard, but I have been using this as a reference: https://github.com/testmoapp/junitxml
<testsuites>
can only ever appear as the root element. It cannot not appear anywhere else in the document.
Some tools choose to use <testsuite>
as the root element. This should be handled equivalent to a <testsuite>
as the single direct child of a root <testsuites>
.
The Jenkins parser previously only supported a <testsuite>
root element. Root <testsuites>
are stripped with a regex. The name of this root <testsuite>
was not needed for disambiguation, since no other <testsuite>
s can appear on the root level. But deeper <testsuite>
s can have siblings, so their name is needed for disambiguation.
We need to keep this naming behavior for backwards compatibility.
With <testsuites>
as the root element, we can have multiple <testsuite>
elements on the level where their names would be ignored. In this case, we do need their names to disambiguate the different hierarchies.
The possible cases are:
<testsuites>
<testsuite name="ignored">
<testsuite name="included">
...
</testsuite>
</testsuite>
<testsuites>
<testsuite name="ignored">
<testsuite name="included">
...
</testsuite>
</testsuite>
<testsuites>
<testsuite name="included A">
<testsuite name="included">
...
</testsuite>
</testsuite>
<testsuite name="included B">
<testsuite name="included">
...
</testsuite>
</testsuite>
<testsuites>
Because this only affects the first or second level elements, it's enough to handle them before the recursion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the confusion comes from the equal parsing of <testsuites>
and <testsuite>
. I can create a distinct record to make it clear that no further <testsuites>
should occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also get that the documentation/naming is confusing. How can we improve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locally tested and worked as expectred.
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
The Jenkins parser concatenates the names of deeply nested
<testsuite>
elements to the<testcase>
names to produce the final test result name, while leaving out the top-level<testsuite>
name.LocalCI currently only uses the
<testcase>
name.Description
The names of
<testsuite>
elements which are not top-level are concatenated like with the Jenkins parser.Additionally, the case of multiple top-level
<testsuite>
elements in a<testsuites>
root element is handled.There the names of the top-level
<testsuite>
elements are included in the result.Fixes #9688.
Steps for Testing
Prerequisites:
Properties.Checked by SmallCheck.Testing filtering in A
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Server
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Refactor