Development: Increase Test Coverage of the Exam Class
#10921
Conversation
github-actions
bot
added
tests
server
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
SamuelRoettgermann
changed the title
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Exam
participant ExamRepository
Client->>Exam: setTitle(title)
Exam-->>Exam: Trim whitespace, require non-null
Client->>Exam: isVisibleToStudents()
Exam-->>Client: Return true/false (never null)
Client->>ExamRepository: filterVisibleExams()
ExamRepository->>Exam: isVisibleToStudents()
Exam-->>ExamRepository: Return true/false
ExamRepository-->>Client: List of visible exams
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. 🔧 ast-grep (0.38.1)src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (1)
src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java (1)
1325-1348: Test logic is correct, but could be more concise.The test properly validates courseName handling for both null and non-null cases. Consider using a parameterized test to reduce code duplication:
- @Test - @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") - void testGetCourseName() throws Exception { - // First we test for an actual name - { - final String testCourseName = "Course Name for Testing"; - final Exam examA = ExamFactory.generateExam(course1); - examA.setCourseName(testCourseName); - assertThat(examA.getCourseName()).isEqualTo(testCourseName); - final URI createdExamURI = request.post("/api/exam/courses/" + course1.getId() + "/exams", examA, HttpStatus.CREATED); - Exam receivedExam = request.get(String.valueOf(createdExamURI), HttpStatus.OK, Exam.class); - assertThat(receivedExam.getCourseName()).isEqualTo(testCourseName); - } - - // Here we test for null - { - final Exam examB = ExamFactory.generateExam(course1); - examB.setCourseName(null); - assertThat(examB.getCourseName()).isNull(); - final URI createdExamURI = request.post("/api/exam/courses/" + course1.getId() + "/exams", examB, HttpStatus.CREATED); - Exam receivedExam = request.get(String.valueOf(createdExamURI), HttpStatus.OK, Exam.class); - assertThat(receivedExam.getCourseName()).isNull(); - } - } + @ParameterizedTest + @MethodSource("provideCourseNames") + @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") + void testGetCourseName(String courseName) throws Exception { + final Exam exam = ExamFactory.generateExam(course1); + exam.setCourseName(courseName); + assertThat(exam.getCourseName()).isEqualTo(courseName); + final URI createdExamURI = request.post("/api/exam/courses/" + course1.getId() + "/exams", exam, HttpStatus.CREATED); + Exam receivedExam = request.get(String.valueOf(createdExamURI), HttpStatus.OK, Exam.class); + assertThat(receivedExam.getCourseName()).isEqualTo(courseName); + } + + private static Stream<Arguments> provideCourseNames() { + return Stream.of( + Arguments.of("Course Name for Testing"), + Arguments.of((String) null) + ); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/exam/domain/Exam.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
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/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.javasrc/main/java/de/tum/cit/aet/artemis/exam/domain/Exam.java
`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/exam/ExamIntegrationTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (12)
src/main/java/de/tum/cit/aet/artemis/exam/domain/Exam.java (4)
447-455: Excellent improvement to method contract and predictability.The change from
Booleanto primitivebooleanforisVisibleToStudents()is a significant improvement that:
- Eliminates null return values, making the API more predictable
- Follows the coding guidelines preference for primitives
- Reduces the need for null checks in calling code
- Provides sensible default behavior (returns
falsewhenvisibleDateis null)
460-468: Consistent improvement aligns with visibility method changes.The
isStarted()method changes mirror theisVisibleToStudents()improvements perfectly. The primitivebooleanreturn type withfalsefallback for null dates provides consistent, predictable behavior across the exam state methods.
476-481: Completes the trilogy of boolean state method improvements.The
resultsPublished()method change maintains consistency with the other two boolean state methods. All three methods now follow the same pattern: primitivebooleanreturn type withfalsefor null date conditions.
178-481:✅ Verification successful
Verify breaking change impact across the codebase.
The return type changes from
Booleantobooleanfor the three state methods represent a breaking change. Any code that previously checked fornullreturn values will need updates.
🏁 Script executed:
#!/bin/bash # Description: Check for potential breaking changes where null checks were used on the modified boolean methods # Expected: Find any code that checks for null on these methods echo "Searching for null checks on the modified boolean methods..." rg -A 3 -B 3 "isVisibleToStudents\(\)\s*==\s*null|null\s*==\s*isVisibleToStudents\(\)" rg -A 3 -B 3 "isStarted\(\)\s*==\s*null|null\s*==\s*isStarted\(\)" rg -A 3 -B 3 "resultsPublished\(\)\s*==\s*null|null\s*==\s*resultsPublished\(\)" echo "Searching for Boolean.TRUE.equals() patterns on these methods..." rg -A 3 -B 3 "Boolean\.TRUE\.equals\(.*isVisibleToStudents\(\)" rg -A 3 -B 3 "Boolean\.TRUE\.equals\(.*isStarted\(\)" rg -A 3 -B 3 "Boolean\.TRUE\.equals\(.*resultsPublished\(\)"Length of output: 4048
No code-level null checks found on Exam methods
I searched for any comparisons against
nullor uses ofBoolean.TRUE.equals(...)on the three modified methods in Exam.java —isVisibleToStudents(),isStarted(), andresultsPublished(). All matches point to calls on StudentExam (e.g. in StudentExamResource.java and StudentExam.java), not on the Exam entity itself.– StudentExamResource and StudentExam use their own wrapper-based logic and are unaffected by the change in Exam.
– No instances ofexam.isStarted() == null,exam.isVisibleToStudents() == null, orBoolean.TRUE.equals(exam.resultsPublished())exist in the codebase.You can safely convert those three getters to
boolean. Just be aware that JSON serialization will now outputfalseinstead ofnullwhen the underlying value is unset—verify any API or frontend clients don’t depend on anullvalue here.src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.java (1)
489-489: Perfect simplification leveraging the domain model improvements.The change from
exam -> Boolean.TRUE.equals(exam.isVisibleToStudents())toExam::isVisibleToStudentsis an excellent improvement that:
- Leverages the new primitive
booleanreturn type fromExam.isVisibleToStudents()- Eliminates the need for
Boolean.TRUE.equals()null-safe comparison- Follows the coding guidelines preference for static member references
- Improves readability and reduces complexity
This change perfectly complements the domain model updates in
Exam.java.src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java (7)
42-44: LGTM!The Jackson imports are correctly added and necessary for the serialization/deserialization tests in the
testQuizExamMaxPointsmethod.
1263-1273: Good test coverage for title trimming!This addition properly tests the trimming behavior mentioned in the PR objectives. The test correctly validates that exam titles with leading and trailing whitespace are trimmed when retrieved.
1281-1324: Comprehensive test coverage for examiner and correction rounds!These tests effectively validate:
- Examiner name persistence
- Default correction rounds behavior when null
- Valid correction round scenarios for both test and real exams
The helper method
testCreateExamWithCorrectionRoundsAndExpectedCreationStatusis a good abstraction that reduces code duplication.
1350-1381: Excellent test coverage for the transient quizExamMaxPoints field!The test comprehensively validates:
- Direct getter/setter functionality
- JSON serialization includes the field
- JSON deserialization properly sets the field
- Various edge cases from 0 to 1000
The parameterized approach with diverse test values ensures robust coverage.
1383-1438: Well-structured tests for the updated boolean visibility methods!These tests effectively validate the domain changes mentioned in the PR summary where methods like
isVisibleToStudents()andisStarted()now return primitivebooleaninstead of nullableBoolean. The tests properly cover:
- Various time offsets for visibility checks
- Null date handling that prevents exam creation
- Both positive and negative test cases
The parameterized approach with
@ValueSourceis clean and efficient.
1441-1514: Excellent comprehensive test for exam end time calculation!This test thoroughly covers all scenarios for
isAfterLatestStudentExamEnd():
- Default behavior with no student exams
- Regular exams where all students have the same working time
- Exams with minor time advantages that don't affect the overall end
- Exams where student time advantages extend beyond the regular end time
The clear separation of test cases with descriptive comments makes the complex business logic easy to understand.
1516-1533: Good edge case coverage for exam archive path validation!The test properly validates the
hasExamArchive()boolean method with:
nullpath →false- Empty string →
false- Valid paths →
trueThe helper method effectively reduces code duplication while maintaining clarity.
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
SamuelRoettgermann
changed the title
Development: Increase Test Coverage of the Exam Class
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
Checklist
General
Server
Motivation and Context
Test coverage should be increased.
Description
Increases test coverage of the Exam class to effectively 100%.
I also made a few, small changes to the Exam class itself, where I changed method return types from boxed to primitive versions and simplified a few lines to assume non-null where it was either impossible to have a null value in the first place, or where, if called with null, would lead to a
NullPointerExceptionanyway, or where thenullreturn value was never used and also doesn't really make sense.Exam.java#titleis already non-nullable, even in the database (runSHOW columns FROM examto verify yourself).I made sure to use appropriate APIs rather than calling the
savefunction where appropriate.Steps for Testing
Run the
ExamIntegrationTestand verify that the tests in there work.Exam Mode Testing
Prerequisites:
2.1. Try to omit the title - you should not be able to create the exam, i.e., hit the "Save" button
2.2. Try to omit the "Visible From" field - you should again not be able to create the exam
2.3. Try to omit the "Start of Working Time" - you should again not be able to create the exam
Code Review
Manual Tests
Exam Mode Test
Test Coverage
Summary by CodeRabbit
Bug Fixes
Tests