Development: Improve programming exercise test coverage
#10765
Conversation
…alVCLocalCIIntegrationTest
github-actions
bot
added
tests
programming
ekayandan
changed the title
Development: Improve Programming Exercise test coverage
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
…se-tests' into chore/improve-programming-exercise-tests
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
…:ls1intum/Artemis into chore/improve-programming-exercise-tests
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||||||||
…:ls1intum/Artemis into chore/improve-programming-exercise-tests
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Code
I left some suggestions for future PRs.
The actual testing code is quite clean and well understandable. For me the Code in ProgrammingExerciseImportTestService is rather hard to understand, I think that the abbreviations and too short variable names make it harder to understand than it would need to be - but this might also be an opinionated topic
Another thing, please link issues properly to PRs https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests. Bought of the linked issues are still open but should be closed, shouldn't they (I did that manually just now)?
If they are linked properly that happens automatically once the PR is merged
...de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseImportTestService.java
Show resolved
Hide resolved
...de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseImportTestService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Mmh, unfortunately one tests seems to fail in the pipeline? - ProgrammingExerciseLocalVCLocalCIIntegrationTest importFromFile_verifyBuildPlansCreated()
https://bamboo.ase.in.tum.de/browse/ARTEMIS-TESTS8121-JAVATEST-28
ekayandan
dismissed stale reviews from marlonnienaber, theblobinthesky, and coderabbitai[bot]
via
b46b3cb
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java (1)
366-399: Good implementation of import test with title modification verification.The test properly verifies that title changes propagate to all repository files by comparing occurrences in extracted directories and zip files. Consider adding the assertion that
newTitleCount != 0to ensure the test is valid when no occurrences are found.assertThat(newTitleCount).isEqualTo(programmingExerciseImportTestService.countOccurrencesInZip(importResult.resource(), oldTitle)); + assertThat(newTitleCount).isGreaterThan(0); assertThat(oldTitleCount).isZero();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
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
src/test/java/de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: server-style
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (9)
src/test/java/de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java (9)
85-89: Good refactoring using the autowired test services.The integration of
ProgrammingExerciseTestServiceandProgrammingExerciseImportTestServicefollows the suggested utility/service pattern from previous review comments, promoting code reuse and better test organization.
142-144: Good delegation to test service for setup.The setup has been properly refactored to delegate user initialization and environment setup to the test service, following the previous recommendation to factor out setup logic into service classes.
158-158: Proper test cleanup using service.The tearDown method correctly uses the test service for cleanup operations. This ensures consistent resource management across tests.
311-339: Well-organized delegation of import error tests.These test methods effectively delegate to the test service to verify various error handling scenarios for importing programming exercises. This pattern improves maintainability by centralizing test logic.
341-345: Good CI failure test.This test correctly validates the system's behavior when failing to create a project in the CI environment, which helps prevent issue regression.
347-352: Excellent use of parameterized testing for date validation.The
@ParameterizedTestwithArgumentsSourcefollows JUnit 5 best practices for testing multiple input combinations, which is ideal for date validation scenarios.
401-414: Clean implementation of basic import test.This test verifies the basic functionality of importing a valid exercise without modifications, focusing on core verification points like exercise properties and course association.
416-461: Comprehensive build plan verification test.This test effectively mocks Docker interactions to verify that build plans are properly created after import. Good practice using try-catch with a detailed error message to help with troubleshooting.
463-491: Good test for sequential test runs feature.The test properly verifies that exercises can be created with sequential test runs enabled. It follows the same pattern as other creation tests while focusing on the specific feature being tested.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/test/java/de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java (1)
161-161: Consider addressing the disabled testThe test on line 161 is still disabled. According to previous review comments, there was an attempt to fix it.
Could you provide an update on the status of this test? If it's intentionally disabled, consider adding a comment explaining why.
🧹 Nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java (2)
370-403: Well-structured test for title replacement during importThis test effectively verifies that when changing a title during import, the new title properly replaces the old one in all repository files. The test covers a specific use case mentioned in issue #7188.
However, you could strengthen the assertion on line 401 by adding a check that
newTitleCountis not zero:- assertThat(newTitleCount).isEqualTo(programmingExerciseImportTestService.countOccurrencesInZip(importResult.resource(), oldTitle)); + assertThat(newTitleCount).isEqualTo(programmingExerciseImportTestService.countOccurrencesInZip(importResult.resource(), oldTitle)) + .isGreaterThan(0);This would guard against the case where both counts are zero, which would pass the current test but indicate a potential issue with the counting mechanism.
451-468: Improve exception handling approachThe current try-catch block catches all exceptions and wraps them in an AssertionError, which can obscure the original exception details.
Consider replacing the generic Exception with more specific exceptions or removing the try-catch block altogether:
- try { // Refresh the exercise to get latest participation data ProgrammingExercise refreshedExercise = programmingExerciseRepository.findWithAllParticipationsAndBuildConfigById(importedExercise.getId()).orElseThrow(); // Verify template build plan TemplateProgrammingExerciseParticipation templateParticipation = templateProgrammingExerciseParticipationRepository .findByProgrammingExerciseId(refreshedExercise.getId()).orElseThrow(); localVCLocalCITestService.testLatestSubmission(templateParticipation.getId(), null, 0, false, 30); // Verify solution build plan SolutionProgrammingExerciseParticipation solutionParticipation = solutionProgrammingExerciseParticipationRepository .findByProgrammingExerciseId(refreshedExercise.getId()).orElseThrow(); localVCLocalCITestService.testLatestSubmission(solutionParticipation.getId(), null, 13, false, 30); - } - catch (Exception e) { - throw new AssertionError("Failed to verify build plans", e); - }This will allow JUnit to handle the exceptions directly and provide more detailed failure information.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java(7 hunks)src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseImportTestService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseImportTestService.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
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
src/test/java/de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java
🧬 Code Graph Analysis (1)
src/test/java/de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java (1)
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java (1)
ProgrammingExerciseFactory(46-472)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (9)
src/test/java/de/tum/cit/aet/artemis/programming/icl/ProgrammingExerciseLocalVCLocalCIIntegrationTest.java (9)
85-89: Good refactoring using autowired test servicesUsing dedicated test services with dependency injection helps improve code organization and reusability. This follows the util_service_factory_pattern guideline.
143-144: Good delegation of test setup to the serviceThis delegation properly separates test setup logic into a reusable component, making the test class more maintainable.
158-158: Good teardown delegationDelegating the teardown logic to the test service ensures proper cleanup between tests, maintaining test isolation.
311-315: Good test modularization for import error casesDelegating test implementations to the
ProgrammingExerciseTestServicepromotes code reuse and improves readability. This pattern is consistently applied across multiple test methods.
347-352: Good use of parameterized testsUsing parameterized tests with an argument provider is an excellent practice for testing multiple invalid date configurations without code duplication. This follows the junit5_features guideline.
366-369: Good documentation linking to GitHub issueThe JavaDoc comment linking to GitHub issue #7188 provides helpful context about which regression this test is preventing.
420-423: Good documentation linking to GitHub issueThe JavaDoc comment linking to GitHub issue #8562 provides helpful context about which regression this test is preventing.
424-469: Comprehensive test for build plan creation verificationThis test thoroughly verifies that build plans are correctly created after an import operation, including mocking Docker interactions and verifying both template and solution build plans.
471-499: Good test for sequential test runsThis test verifies the creation of a programming exercise with sequential test runs enabled, which improves test coverage as mentioned in the PR objectives.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary |
github-project-automation
bot
moved this from Ready For Review
to Merged
in Artemis Development
Checklist
General
Server
Motivation and Context
Programming Exercise: Changing the short name while importing breaks tests #7188 andProgramming exercises: Build results not available after file import #8562 from happening againDescription
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Code Review
Test Coverage
Summary by CodeRabbit