Iris: Add integration test for rewriting and consistency pipeline
#10931
Conversation
github-actions
bot
added
tests
core
cremertim
added
ready for review
and removed
tests
core
cremertim
changed the title
IRIS: Add test for rewriting pipelineIris: Add test for rewriting pipeline
WalkthroughNew integration tests for the Pyris rewriting pipeline and consistency check were added, verifying functionality and role-based access control. A utility class for mock LLM cost data was introduced and used to replace a local helper method. A mock provider method was renamed and its endpoint updated, and a new mock method for consistency check responses was added. Changes
Sequence Diagram(s)sequenceDiagram
participant Instructor
participant TestClient
participant IrisRequestMockProvider
participant Websocket
Instructor->>TestClient: Send rewriting request (text, variant)
TestClient->>IrisRequestMockProvider: Mock rewriting pipeline response
IrisRequestMockProvider-->>TestClient: Return mocked response
TestClient->>Websocket: Send intermediate stage message (no result)
TestClient->>Websocket: Send final stage message (with result)
Websocket-->>Instructor: Deliver messages
sequenceDiagram
participant Instructor
participant TestClient
participant IrisRequestMockProvider
participant IrisConsistencyCheckService
participant Websocket
Instructor->>TestClient: Send consistency check request
TestClient->>IrisRequestMockProvider: Mock consistency check response
IrisRequestMockProvider-->>TestClient: Return HTTP 202 Accepted
IrisConsistencyCheckService->>Websocket: Send staged status updates
Websocket-->>Instructor: Deliver consistency check messages
Suggested labels
Suggested reviewers
✨ 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: 0
🧹 Nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.java (1)
89-98: Good access control test but consider enhancing verification.The test properly verifies that students cannot access the rewriting pipeline. However, the mock setup with an empty consumer doesn't add value since the request should be blocked before reaching the pipeline.
Consider removing the unnecessary mock setup since the request should be forbidden at the controller level:
- irisRequestMockProvider.mockRewritingPipelineResponse(dto -> { - });src/test/java/de/tum/cit/aet/artemis/iris/utils/IrisLLMMock.java (1)
12-14: Consider extracting magic numbers as constants for better maintainability.While the current implementation works well, extracting the magic numbers would improve readability and maintainability.
Consider extracting constants for better clarity:
+ private static final int MOCK_DATA_COUNT = 5; + private static final String MOCK_MODEL_NAME = "test-llm"; + private static final String MOCK_PIPELINE_ID = "IRIS_CHAT_EXERCISE_MESSAGE"; + public static List<LLMRequest> getMockLLMCosts() { List<LLMRequest> costs = new ArrayList<>(); - for (int i = 0; i < 5; i++) { - costs.add(new LLMRequest("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, "IRIS_CHAT_EXERCISE_MESSAGE")); + for (int i = 0; i < MOCK_DATA_COUNT; i++) { + costs.add(new LLMRequest(MOCK_MODEL_NAME, i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, MOCK_PIPELINE_ID)); } return costs; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/utils/IrisLLMMock.java(1 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/iris/IrisChatTokenTrackingIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.javasrc/test/java/de/tum/cit/aet/artemis/iris/utils/IrisLLMMock.javasrc/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.java
🧬 Code Graph Analysis (2)
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1)
src/test/java/de/tum/cit/aet/artemis/iris/utils/IrisLLMMock.java (1)
IrisLLMMock(8-17)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.java (3)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-498)src/main/webapp/app/app.constants.ts (1)
PROFILE_IRIS(52-52)src/test/java/de/tum/cit/aet/artemis/iris/utils/IrisLLMMock.java (1)
IrisLLMMock(8-17)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Codacy Static Code Analysis
- 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: client-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (5)
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (1)
200-212: LGTM! Improved method naming and endpoint consistency.The refactoring improves the method name to be more descriptive and follows the naming pattern of other mock methods in the class. The parameter name change from
executionDTOConsumertoresponseConsumeris more concise and clear. The endpoint simplification from/rewriting/faq/runto/rewriting/runappears to align with API changes.src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.java (2)
31-53: Well-structured test class setup with proper naming and initialization.The test class follows excellent practices:
- Descriptive class name and test prefix
- Proper Spring profile configuration
- Clean setup with course and users initialization
- Iris activation for testing context
55-87: Comprehensive integration test with proper websocket verification.The test method effectively validates:
- Mock pipeline response with input verification
- Proper HTTP status handling
- Websocket message sequence verification using ArgumentCaptor
- Stage progression and result validation
The test follows the coding guidelines with descriptive naming and proper assertThat usage.
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1)
3-3: Excellent refactoring to centralize mock data generation.The import of the static
getMockLLMCosts()method from the new utility class improves code organization by following the DRY principle. This centralizes the mock LLM cost data generation for reuse across multiple test classes.src/test/java/de/tum/cit/aet/artemis/iris/utils/IrisLLMMock.java (1)
8-17: Well-designed utility class following good practices.The utility class design is excellent:
- Final class prevents inheritance
- Static method for utility functionality
- Descriptive method name
- Consistent mock data generation
The deterministic parameter calculations ensure reproducible test results, which is important for reliable testing.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
cremertim
changed the title
Iris: Add test for rewriting pipelineIris: Add test for rewriting and consistency pipeline
github-actions
bot
added
tests
core
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.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/iris/PyrisConcistencyCheckIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java
🧠 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java (1)
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-10-15T11:33:17.915Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
🧬 Code Graph Analysis (1)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java (4)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-498)src/main/webapp/app/app.constants.ts (1)
PROFILE_IRIS(52-52)src/test/java/de/tum/cit/aet/artemis/iris/utils/IrisLLMMock.java (1)
IrisLLMMock(8-17)src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.java (1)
Profile(31-98)
⏰ 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-tests
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (8)
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (3)
41-41: LGTM!Import addition is necessary for the new consistency check mock method.
201-213: LGTM!The method rename from
mockRunRewritingResponseAndtomockRewritingPipelineResponseimproves naming consistency and the parameter name change enhances clarity. The implementation follows the established pattern.
215-223: LGTM!The new
mockProgrammingConsistencyCheckResponsemethod follows the same pattern as other mock methods in this class. The endpoint URL/inconsistency-check/runand HTTP 202 response are appropriate for the consistency check pipeline.src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java (5)
4-4: LGTM!Good use of the utility class
IrisLLMMock.getMockLLMCosts()to provide mock LLM cost data for testing.
56-66: LGTM!The test setup properly creates users with different roles, sets up a programming exercise, and activates Iris for the course. This follows the established pattern from other integration tests and adheres to the coding guidelines.
68-105: LGTM!Comprehensive integration test that verifies:
- Instructor role access and permissions
- DTO conversion and request payload validation
- Websocket messaging with staged updates
- Proper handling of status updates including intermediate and final results
The test follows best practices with proper mocking, assertions, and timeout handling.
107-125: LGTM!Proper role-based access control tests ensuring that students and tutors receive forbidden (403) responses when attempting to trigger consistency checks. This enforces the security requirements correctly.
127-132: LGTM!The helper method creates appropriate test data with hardcoded values which is suitable for integration testing. The DTO includes all necessary fields for comprehensive testing.
src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
cremertim
changed the title
Iris: Add test for rewriting and consistency pipelineIris: Add integration test for rewriting and consistency pipeline
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/iris/utils/IrisLLMMock.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/iris/PyrisRewritingIntegrationTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
see my comments and I agree with all change requests from @florian-glombik
src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java
Outdated
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
Checklist
General
Server
Motivation and Context
In a previous PR, i introduced the Rewriting of FAQs and Problem Statements. However, those functionalities have not been coverd by the server test so far. I made sure to add the tests now.
@FelixTJDietrich and @wasnertobias introduced a consistency check for programming exercises, i also added some test cases for this pipeline
Description
This PR adds integration test for both, the rewriting and the consistency pipeline
Steps for Testing
Only code review necessary, this add some test cases
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
New Features
Refactor