Programming exercises: Concatenate testsuite names with testcase names in LocalCI
#9691
Conversation
github-actions
bot
added
tests
server
magaupp
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
magaupp
changed the title
Programming exercises: Concatenate testsuite names with testcase namesProgramming exercises: Concatenate testsuite names with testcase names in LocalCI
WalkthroughThe pull request introduces substantial modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.6.0)src/main/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParser.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/test/java/de/tum/cit/aet/artemis/buildagent/service/TestResultXmlParserTest.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
testsuiteelements are prepended to thetestcasename with dots."- "A singular top-level
testsuitename is not included in the test case names."- "If multiple top-level
testsuiteelements 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
Outdated
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.
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.
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
flbrgit
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
sachmii
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
…ed-testsuite-names
sachmii
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
|
Looks like
@b-fein Any idea what's happening here? |
…ed-testsuite-names
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 ATestserver 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
Summary by CodeRabbit
New Features
Bug Fixes
Refactor