Hyperion: Add AI code generation assistance for template, solution, and tests repositories
#11405
Conversation
github-actions
bot
added
tests
server
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Hey @luis-gasparschroeder, looks really good already :)
I found some places for potential improvements.
Some question from my side: How does the previous state of the repository get accounted for? Especially for when build issues arise? Do you just discard all and regenerate? It is not 100% clear to me
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionRepositoryStructureService.java
Outdated
Show resolved
Hide resolved
...a/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationStrategy.java
Outdated
Show resolved
Hide resolved
...a/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationStrategy.java
Outdated
Show resolved
Hide resolved
.../java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionSolutionRepository.java
Outdated
Show resolved
Hide resolved
.../java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResource.java
Outdated
Show resolved
Hide resolved
.../cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionService.java
Outdated
Show resolved
Hide resolved
.../cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionService.java
Outdated
Show resolved
Hide resolved
...tor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.ts
Outdated
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
…rciseContextRendererService
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
The code generation does not delete files at this point in time. Consequently, the pre-existing files remain. This functionality can be added in a follow-up PR |
luis-gasparschroeder
dismissed
coderabbitai[bot]’s stale review
via
da81272
luis-gasparschroeder
dismissed
coderabbitai[bot]’s stale review
via
236bbe0
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionProgrammingExerciseContextRendererServiceTest.java (4)
17-24: Consider using static mocks per coding guidelines.The coding guidelines specify
mock_strategy: static_mocksfor this path. Consider replacing@Mockannotations with explicitMockito.mock()calls in the@BeforeEachmethod for consistency with project conventions.Apply this diff:
- @Mock - private RepositoryService repositoryService; - - @Mock - private HyperionProgrammingLanguageContextFilterService languageFilter; - - @Mock - private GitService gitService; + private RepositoryService repositoryService; + private HyperionProgrammingLanguageContextFilterService languageFilter; + private GitService gitService;And update the setup method:
@BeforeEach void setup() { - MockitoAnnotations.openMocks(this); + repositoryService = Mockito.mock(RepositoryService.class); + languageFilter = Mockito.mock(HyperionProgrammingLanguageContextFilterService.class); + gitService = Mockito.mock(GitService.class); contextRendererService = new HyperionProgrammingExerciseContextRendererService(repositoryService, languageFilter);As per coding guidelines.
56-63: Strengthen assertion for null problem statement test.The test verifies graceful handling but only asserts
isNotNull(), which doesn't validate the actual behavior. Consider verifying that the rendered context either contains a placeholder/default message or follows the expected format even with a null problem statement.Example enhancement:
@Test void renderContext_withNullProblemStatement_handlesGracefully() { exercise.setProblemStatement(null); String result = contextRendererService.renderContext(exercise); - assertThat(result).isNotNull(); + assertThat(result).isNotNull().isNotEmpty(); + // Or verify it contains expected structure/placeholderAs per coding guidelines:
assert_specificity: true.
66-66: Remove or specify exception in test signature.The
throws Exceptiondeclaration is too broad and likely unnecessary. If the method can throw specific checked exceptions, declare them explicitly; otherwise, remove the throws clause entirely to improve test clarity.- void getExistingSolutionCode_withNullRepositoryUri_returnsWarningMessage() throws Exception { + void getExistingSolutionCode_withNullRepositoryUri_returnsWarningMessage() {As per coding guidelines: tests should be specific and clear about expected behavior.
72-88: Strengthen assertions for edge-case tests.Tests
renderContext_withNullProgrammingLanguage_handlesGracefullyandrenderContext_withEmptyProblemStatement_handlesGracefullyonly assertisNotNull(), which merely confirms no exception was thrown. Consider verifying the actual rendered output structure, default values, or expected placeholders to ensure correct behavior under these edge cases.Example enhancement:
@Test void renderContext_withNullProgrammingLanguage_handlesGracefully() { exercise.setProgrammingLanguage(null); String result = contextRendererService.renderContext(exercise); - assertThat(result).isNotNull(); + assertThat(result).isNotNull().isNotEmpty(); + // Verify expected format or default behavior } @Test void renderContext_withEmptyProblemStatement_handlesGracefully() { exercise.setProblemStatement(""); String result = contextRendererService.renderContext(exercise); - assertThat(result).isNotNull(); + assertThat(result).isNotNull().isNotEmpty(); + // Verify expected handling of empty vs null problem statements }As per coding guidelines:
assert_specificity: true— assertions should verify actual behavior, not just absence of errors.src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionProgrammingLanguageContextFilterServiceTest.java (2)
70-86: Consider adding absence assertions for excluded files.The test properly validates inclusion of Java source files but could be more explicit by also asserting the absence of excluded files (e.g.,
target/classes,build.gradle,.idea,node_modules). This matches the pattern infilter_withJavaLanguage_filtersCorrectly(lines 36-38) and makes the test intent clearer.Apply this diff:
assertThat(result).hasSize(2); assertThat(result).containsKey("src/main/java/com/example/Service.java"); assertThat(result).containsKey("src/main/java/com/example/Controller.java"); + assertThat(result).doesNotContainKey("target/classes/Service.class"); + assertThat(result).doesNotContainKey("build.gradle"); + assertThat(result).doesNotContainKey(".idea/workspace.xml"); + assertThat(result).doesNotContainKey("node_modules/package/index.js"); }
88-103: Consider adding absence assertions for excluded files.Similar to the previous test, explicitly asserting the absence of non-Java files would make the filtering behavior more obvious and align with the pattern in
filter_withJavaLanguage_filtersCorrectly.Apply this diff:
assertThat(result).hasSize(2); assertThat(result).containsKey("src/main/java/Main.java"); assertThat(result).containsKey("src/test/java/Test.java"); + assertThat(result).doesNotContainKey("style.css"); + assertThat(result).doesNotContainKey("script.js"); + assertThat(result).doesNotContainKey("data.json"); + assertThat(result).doesNotContainKey("config.xml"); }src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java (1)
10-10: Add explicit package-private visibility modifier.The test class should explicitly declare package-private visibility (
/* package */) to align with Java coding conventions and make the intended visibility clear.Apply this diff:
-class HyperionPromptTemplateServiceTest { +/* package */ class HyperionPromptTemplateServiceTest {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionProgrammingExerciseContextRendererServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionProgrammingLanguageContextFilterServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionProgrammingExerciseContextRendererServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionProgrammingLanguageContextFilterServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: bean-instantiation-check
🔇 Additional comments (6)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionProgrammingLanguageContextFilterServiceTest.java (6)
1-11: LGTM!Package declaration and imports are clean and follow coding guidelines. Correctly uses assertj's
assertThatfor assertions.
13-20: LGTM!Test class structure is clean and follows best practices. Direct instantiation of the service in
@BeforeEachis appropriate for a stateless utility, avoiding unnecessary Spring context overhead.
22-39: LGTM!Test properly validates Java source file filtering with realistic paths. Assertions are specific and comprehensive, correctly verifying both inclusion and exclusion.
41-52: LGTM!Test correctly validates fallback behavior for unsupported languages. Using
containsExactlyInAnyOrderEntriesOfis an appropriate assertion to verify all files pass through unfiltered.
54-61: LGTM!Edge case test for empty input is simple, focused, and uses the appropriate assertion.
63-68: LGTM!Null input edge case is properly tested, documenting the service's null-safety behavior.
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.html (2)
135-139: Restore an accessible label while the spinner shows.The spinner-only fallback still has no accessible name despite earlier feedback. Add a translated hidden label (or aria-label) so assistive tech can announce the control.
- @if (isGeneratingCode()) { - <button class="btn btn-outline-primary" disabled> - <fa-icon [icon]="faSpinner" animation="spin" /> - </button> + @if (isGeneratingCode()) { + <button class="btn btn-outline-primary" disabled> + <fa-icon [icon]="faSpinner" animation="spin" /> + <span class="visually-hidden" jhiTranslate="artemisApp.programmingExercise.codeGeneration.creationAssistance"></span> + </button> }
142-152: Remove hardcoded fallback text from translated labels.
jhiTranslatealready injects the localized copy; keeping the English strings defeats the point of translation and was flagged earlier for the dropdown item.- <span class="ms-2" jhiTranslate="artemisApp.programmingExercise.codeGeneration.creationAssistance">Creation Assistance</span> + <span class="ms-2" jhiTranslate="artemisApp.programmingExercise.codeGeneration.creationAssistance"></span> ... - Generate Code
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/main/resources/config/application-artemis.ymlis excluded by!**/*.yml
📒 Files selected for processing (5)
package.json(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java(1 hunks)src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.html(4 hunks)src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.ts(4 hunks)supporting_scripts/code-coverage/module-coverage-client/check-client-module-coverage.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- supporting_scripts/code-coverage/module-coverage-client/check-client-module-coverage.mjs
🧰 Additional context used
📓 Path-based instructions (3)
src/main/webapp/**/*.ts
⚙️ CodeRabbit configuration file
Files:
src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.ts
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java
src/main/webapp/**/*.html
⚙️ CodeRabbit configuration file
@if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
Files:
src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.html
🧬 Code graph analysis (1)
src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.ts (3)
src/main/webapp/app/app.constants.ts (1)
MODULE_FEATURE_HYPERION(40-40)src/main/webapp/app/shared/service/base-api-http.service.ts (1)
request(40-70)src/main/webapp/app/openapi/model/codeGenerationRequestDTO.ts (3)
CodeGenerationRequestDTO(15-20)RepositoryTypeEnum(22-26)RepositoryTypeEnum(27-27)
🪛 GitHub Actions: CodeQL
src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java
[error] 141-141: cannot find symbol: @PostConstruct. Ensure the appropriate annotation is on the classpath (e.g., javax.annotation-api) or proper import is present.
[error] 159-159: cannot find symbol: @PreDestroy. Ensure the appropriate annotation is on the classpath (e.g., javax.annotation-api) or proper import is present.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: bean-instantiation-check
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java (3)
119-121: LGTM!The constructor properly delegates to the superclass and supports the lifecycle initialization hooks added below.
123-130: LGTM!The accessor method is well-documented and provides necessary access to the default branch configuration for dependent components.
159-163: Missing import already flagged above.The
@PreDestroyannotation import is missing (flagged in the previous comment). Once the import is added, this cleanup method properly delegates to the superclass for resource cleanup.
src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java
Outdated
Show resolved
Hide resolved
...r/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.html
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.java (1)
160-163: Misleading cleanupRepository test still unverifiedThe test name promises a GitService interaction, but nothing is stubbed or verified—
gitService.getOrCheckoutRepositoryon a bare mock returns null, so you invokecleanupRepositorywith a null repo and assert nothing. Either rename the test to reflect the actual expectation (no exception) or stub/verify the GitService + repository calls carried out insidecleanupRepository. This is the same gap that was raised earlier.
🧹 Nitpick comments (7)
src/test/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResourceTest.java (2)
82-99: Extract magic number to constant.The hardcoded value
3for maximum attempts appears throughout the tests. This should be extracted as a constant to ensure consistency and make future updates easier if the max attempts value changes.Add a constant at the class level:
class HyperionCodeGenerationResourceTest { + private static final int MAX_GENERATION_ATTEMPTS = 3; + @Mock private UserRepository userRepository;Then update the assertions:
- assertThat(response.getBody().attempts()).isEqualTo(3); + assertThat(response.getBody().attempts()).isEqualTo(MAX_GENERATION_ATTEMPTS);Apply this pattern to all occurrences in lines 98, 115, 274, 286, and 298.
Also applies to: 101-116
229-347: Consider parameterizing repetitive test patterns.The tests for
buildGenerationResponse(lines 229-299) andbuildSuccessMessage(lines 301-347) follow identical patterns with different inputs and expected outputs. Once the tests are refactored to test through the public API (per previous comment), consider using JUnit 5's@ParameterizedTestwith@CsvSourceor@MethodSourceto reduce duplication.Example structure:
@ParameterizedTest @CsvSource({ "SOLUTION, true, 'Solution code generated successfully'", "TEMPLATE, true, 'Template code generated successfully'", "TESTS, true, 'Test code generated successfully'" }) void generateCode_successCases_returnsExpectedMessages( RepositoryType type, boolean success, String expectedMessage) { // Single test method covering all cases }src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationRequestDTOTest.java (3)
31-59: Consolidate duplicate validation tests using parameterized tests.These three tests (
testValidCodeGenerationRequestDTO,testCodeGenerationRequestDTO_WithTemplateRepository,testCodeGenerationRequestDTO_WithTestsRepository) have identical logic with only theRepositoryTypeenum value changing.As per coding guidelines
Replace with a single parameterized test:
+import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + -@Test -void testValidCodeGenerationRequestDTO() { - CodeGenerationRequestDTO dto = new CodeGenerationRequestDTO(RepositoryType.SOLUTION); - - Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); - - assertThat(violations).isEmpty(); - assertThat(dto.repositoryType()).isEqualTo(RepositoryType.SOLUTION); -} - -@Test -void testCodeGenerationRequestDTO_WithTemplateRepository() { - CodeGenerationRequestDTO dto = new CodeGenerationRequestDTO(RepositoryType.TEMPLATE); - - Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); - - assertThat(violations).isEmpty(); - assertThat(dto.repositoryType()).isEqualTo(RepositoryType.TEMPLATE); -} - -@Test -void testCodeGenerationRequestDTO_WithTestsRepository() { - CodeGenerationRequestDTO dto = new CodeGenerationRequestDTO(RepositoryType.TESTS); - - Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); - - assertThat(violations).isEmpty(); - assertThat(dto.repositoryType()).isEqualTo(RepositoryType.TESTS); -} +@ParameterizedTest +@EnumSource(RepositoryType.class) +void testCodeGenerationRequestDTO_AllValidTypes(RepositoryType repositoryType) { + CodeGenerationRequestDTO dto = new CodeGenerationRequestDTO(repositoryType); + + Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); + + assertThat(violations).isEmpty(); + assertThat(dto.repositoryType()).isEqualTo(repositoryType); +}
91-100: Remove redundant test.
testCodeGenerationRequestDTO_JsonIncludeNonEmptyduplicates the coverage already provided bytestCodeGenerationRequestDTO_JsonSerialization(lines 72-79). Both verify that a non-null enum value is included in JSON output.Apply this diff:
-@Test -void testCodeGenerationRequestDTO_JsonIncludeNonEmpty() throws JsonProcessingException { - CodeGenerationRequestDTO dto = new CodeGenerationRequestDTO(RepositoryType.TESTS); - - String json = objectMapper.writeValueAsString(dto); - - // Should include repositoryType since it's not empty - assertThat(json).contains("repositoryType"); - assertThat(json).contains("TESTS"); -} -
127-140: Remove unnecessary null check.Line 135 checks
if (repositoryType != null), butRepositoryType.values()never yields null elements. Enum arrays contain only non-null enum constants.Apply this diff:
@Test void testCodeGenerationRequestDTO_AllRepositoryTypes() { for (RepositoryType repositoryType : RepositoryType.values()) { CodeGenerationRequestDTO dto = new CodeGenerationRequestDTO(repositoryType); assertThat(dto.repositoryType()).isEqualTo(repositoryType); - // Validate that non-null repository types pass validation - if (repositoryType != null) { - Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); - assertThat(violations).isEmpty(); - } + Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); + assertThat(violations).isEmpty(); } }src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationResultDTOTest.java (1)
88-96: Prefer structural JSON assertionsRelying on substring checks (
contains("true"),contains("2")) makes the test fragile to formatting changes and unrelated value occurrences. Parse the JSON (e.g.,readTree+assertThat(node.get("success").asBoolean()).isTrue()) to assert field values directly.src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java (1)
19-31: Remove unusedtempDirand temporary file creation.The
@TempDirfield and temporary file creation (lines 30–31) are declared but never used in the test. The temporary file path is created but the test simulates logic manually instead of loading from the file.Apply this diff to remove unused code:
- @TempDir - Path tempDir; - @BeforeEach void setup() { templateService = new HyperionPromptTemplateService();And in
render_withValidStringVariables_replacesPlaceholders:@Test void render_withValidStringVariables_replacesPlaceholders() throws IOException { - // Create a temporary template file - Path templatePath = tempDir.resolve("test-template.txt"); - Files.writeString(templatePath, "Hello {{name}}, your score is {{score}}!"); - Map<String, String> variables = Map.of("name", "John", "score", "95");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationRequestDTOTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationResultDTOTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResourceTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/test/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResourceTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationRequestDTOTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationResultDTOTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.java
🧬 Code graph analysis (1)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.java (2)
src/main/java/de/tum/cit/aet/artemis/programming/domain/Repository.java (1)
Repository(19-107)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (1)
LocalVCRepositoryUri(17-318)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 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
- GitHub Check: bean-instantiation-check
🔇 Additional comments (3)
src/test/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResourceTest.java (2)
1-59: LGTM!The test setup follows best practices: proper use of Mockito field-level mocking with
MockitoAnnotations.openMocks(this), JUnit 5 annotations, AssertJ assertions, and fixed test data initialization.
61-80: LGTM!The public API tests properly verify the
generateCode()method's behavior across success, failure, null result, exception, and exam exercise scenarios. Good use of mocks and assertions.Also applies to: 349-398
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java (1)
180-236: LGTM! Error-case tests correctly validate service behavior.These tests properly call the actual service methods (
templateService.render()andtemplateService.renderObject()) and verify that:
- Nonexistent templates throw
RuntimeExceptionwith "Failed to load template" message- Null resource paths throw
RuntimeException- Invalid resource paths throw
RuntimeException- Null variables throw
RuntimeExceptionThe assertions are specific and use AssertJ as required by the coding guidelines.
src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationRequestDTOTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResourceTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResourceTest.java (1)
118-347: Refactor tests to avoid testing private methods via reflection.The tests use
ReflectionTestUtils.invokeMethod()28 times to test private methods directly. As noted in the previous review, this is a maintainability anti-pattern that breaks encapsulation and creates brittle tests.Test these behaviors through the public
generateCode()API instead. For example:-@Test -void validateGenerationRequest_withNegativeExerciseId_throwsException() { - CodeGenerationRequestDTO request = new CodeGenerationRequestDTO(RepositoryType.SOLUTION); - assertThatThrownBy(() -> ReflectionTestUtils.invokeMethod(resource, "validateGenerationRequest", -1L, request)) - .isInstanceOf(BadRequestAlertException.class) - .hasMessageContaining("Exercise ID must be positive"); -} +@Test +void generateCode_withNegativeExerciseId_throwsBadRequest() { + CodeGenerationRequestDTO request = new CodeGenerationRequestDTO(RepositoryType.SOLUTION); + assertThatThrownBy(() -> resource.generateCode(-1L, request)) + .isInstanceOf(BadRequestAlertException.class) + .hasMessageContaining("Exercise ID must be positive"); +}Based on coding guidelines
🧹 Nitpick comments (7)
src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationResultDTO.java (2)
84-96: Consider more precise JSON assertions.The JSON serialization tests use loose
contains()checks that only verify substring presence. This approach:
- Doesn't validate JSON structure or exact field values
- Could pass incorrectly if additional fields or formatting changes occur
For more robust tests:
- Lines 84-96: Parse the JSON and assert on the deserialized object's fields
- Lines 110-120: Verify field values match expectations, not just presence
- Lines 122-132: Parse JSON and explicitly verify the
messagefield is absent usingassertThat(jsonNode.has("message")).isFalse()Example refactor for lines 122-132:
@Test void testCodeGenerationResultDTO_JsonWithEmptyMessage() throws JsonProcessingException { CodeGenerationResultDTO dto = new CodeGenerationResultDTO(false, "", 2); String json = objectMapper.writeValueAsString(dto); + JsonNode jsonNode = objectMapper.readTree(json); - // Empty string should be excluded due to JsonInclude.NON_EMPTY - assertThat(json).contains("success"); - assertThat(json).doesNotContain("message"); - assertThat(json).contains("attempts"); + // Empty string should be excluded due to JsonInclude.NON_EMPTY + assertThat(jsonNode.has("success")).isTrue(); + assertThat(jsonNode.get("success").asBoolean()).isFalse(); + assertThat(jsonNode.has("message")).isFalse(); + assertThat(jsonNode.has("attempts")).isTrue(); + assertThat(jsonNode.get("attempts").asInt()).isEqualTo(2); }Also applies to: 110-120, 122-132
189-222: Optional: parameterize common scenario tests to reduce duplication.These tests verify domain-specific success/failure messages but duplicate the assertion logic from earlier tests (lines 20-36). Consider using JUnit 5's
@ParameterizedTestto reduce boilerplate:@ParameterizedTest @CsvSource({ "true, 'Solution code generated successfully and compiles without errors.', 1", "true, 'Template code generated successfully and compiles without errors.', 2", "true, 'Test code generated successfully and compiles without errors.', 1" }) void testCodeGenerationResultDTO_CommonSuccessScenarios(boolean success, String message, int attempts) { CodeGenerationResultDTO dto = new CodeGenerationResultDTO(success, message, attempts); assertThat(dto.success()).isTrue(); assertThat(dto.message()).isEqualTo(message); assertThat(dto.attempts()).isEqualTo(attempts); }Similar approach for failure scenarios. This reduces maintenance and makes the test data more visible.
src/test/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResourceTest.java (1)
349-365: Consider using @ParameterizedTest for repository type iteration.The loop-based test works but has a drawback: if one repository type fails, the remaining types won't be tested. A
@ParameterizedTestwith@EnumSourcewould provide better test isolation and clearer failure reporting.Apply this refactor:
-@Test -void generateCode_withAllSupportedRepositoryTypes_processesCorrectly() { - for (RepositoryType repositoryType : new RepositoryType[] { RepositoryType.SOLUTION, RepositoryType.TEMPLATE, RepositoryType.TESTS }) { - CodeGenerationRequestDTO request = new CodeGenerationRequestDTO(repositoryType); - Result mockResult = mock(Result.class); - - when(userRepository.getUserWithGroupsAndAuthorities()).thenReturn(testUser); - when(programmingExerciseRepository.findByIdWithTemplateAndSolutionParticipationElseThrow(1L)).thenReturn(testExercise); - when(codeGenerationExecutionService.generateAndCompileCode(testExercise, testUser, repositoryType)).thenReturn(mockResult); - when(mockResult.isSuccessful()).thenReturn(true); - - ResponseEntity<CodeGenerationResultDTO> response = resource.generateCode(1L, request); - - assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); - assertThat(response.getBody().success()).isTrue(); - } -} +@ParameterizedTest +@EnumSource(value = RepositoryType.class, names = { "SOLUTION", "TEMPLATE", "TESTS" }) +void generateCode_withSupportedRepositoryType_processesSuccessfully(RepositoryType repositoryType) { + CodeGenerationRequestDTO request = new CodeGenerationRequestDTO(repositoryType); + Result mockResult = mock(Result.class); + + when(userRepository.getUserWithGroupsAndAuthorities()).thenReturn(testUser); + when(programmingExerciseRepository.findByIdWithTemplateAndSolutionParticipationElseThrow(1L)).thenReturn(testExercise); + when(codeGenerationExecutionService.generateAndCompileCode(testExercise, testUser, repositoryType)).thenReturn(mockResult); + when(mockResult.isSuccessful()).thenReturn(true); + + ResponseEntity<CodeGenerationResultDTO> response = resource.generateCode(1L, request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); + assertThat(response.getBody().success()).isTrue(); +}Don't forget to add the import:
import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource;src/test/java/de/tum/cit/aet/artemis/hyperion/CodeGenerationResultDTOTest.java (1)
68-74: Clarify negative attempts handling
The DTO and tests accept anyint, including negatives; enforce non-negative validation inCodeGenerationResultDTOor document that negative values are intentional error indicators.src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationRequestDTO.java (3)
31-59: Consider parameterizing repetitive tests.The three tests for SOLUTION, TEMPLATE, and TESTS repository types follow identical structure and could be consolidated into a single parameterized test using
@ParameterizedTestwith@EnumSource(RepositoryType.class)or@ValueSource. This reduces duplication and improves maintainability.Example refactor:
- @Test - void testValidCodeGenerationRequestDTO() { - CodeGenerationRequestDTO dto = new CodeGenerationRequestDTO(RepositoryType.SOLUTION); - - Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); - - assertThat(violations).isEmpty(); - assertThat(dto.repositoryType()).isEqualTo(RepositoryType.SOLUTION); - } - - @Test - void testCodeGenerationRequestDTO_WithTemplateRepository() { - CodeGenerationRequestDTO dto = new CodeGenerationRequestDTO(RepositoryType.TEMPLATE); - - Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); - - assertThat(violations).isEmpty(); - assertThat(dto.repositoryType()).isEqualTo(RepositoryType.TEMPLATE); - } - - @Test - void testCodeGenerationRequestDTO_WithTestsRepository() { - CodeGenerationRequestDTO dto = new CodeGenerationRequestDTO(RepositoryType.TESTS); - - Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); - - assertThat(violations).isEmpty(); - assertThat(dto.repositoryType()).isEqualTo(RepositoryType.TESTS); - } + @ParameterizedTest + @EnumSource(RepositoryType.class) + void testValidCodeGenerationRequestDTO(RepositoryType repositoryType) { + CodeGenerationRequestDTO dto = new CodeGenerationRequestDTO(repositoryType); + + Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); + + assertThat(violations).isEmpty(); + assertThat(dto.repositoryType()).isEqualTo(repositoryType); + }
91-100: Test doesn't verify NON_EMPTY exclusion behavior.The test verifies that a non-null/non-empty
repositoryTypeis included in JSON, but doesn't test the actual@JsonInclude(JsonInclude.Include.NON_EMPTY)behavior—which is to exclude null or empty values. Add a test case withnullto verify exclusion, or document that the current test only validates inclusion of present values.Example test for exclusion behavior:
@Test void testCodeGenerationRequestDTO_JsonIncludeExcludesNull() throws JsonProcessingException { // Note: This would require a constructor or builder that allows null, // or testing at the field level if the DTO evolves to have optional fields // For now, document that NON_EMPTY only applies when fields are optional }Alternatively, if
repositoryTypeis always required (@NotNull), consider whether@JsonInclude(JsonInclude.Include.NON_EMPTY)is necessary at the class level.
127-140: Remove redundant null check and overlapping test logic.Line 135's null check is unnecessary—
RepositoryType.values()returns non-null enum constants. Additionally, this test overlaps with the earlier individual tests (lines 31–59) and could be removed if those are parameterized.Apply this diff to remove the unnecessary check:
@Test void testCodeGenerationRequestDTO_AllRepositoryTypes() { for (RepositoryType repositoryType : RepositoryType.values()) { CodeGenerationRequestDTO dto = new CodeGenerationRequestDTO(repositoryType); assertThat(dto.repositoryType()).isEqualTo(repositoryType); - // Validate that non-null repository types pass validation - if (repositoryType != null) { - Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); - assertThat(violations).isEmpty(); - } + Set<ConstraintViolation<CodeGenerationRequestDTO>> violations = validator.validate(dto); + assertThat(violations).isEmpty(); } }Consider removing this test entirely if you adopt the parameterized test suggestion for lines 31–59, as it would be redundant.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/test/java/de/tum/cit/aet/artemis/hyperion/CodeGenerationRequestDTOTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/CodeGenerationResultDTOTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationRequestDTO.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationResultDTO.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResourceTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.java
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationResultDTO.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationRequestDTO.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResourceTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/CodeGenerationResultDTOTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/CodeGenerationRequestDTOTest.java
🧬 Code graph analysis (3)
src/test/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResourceTest.java (3)
src/main/webapp/app/openapi/model/codeGenerationRequestDTO.ts (1)
CodeGenerationRequestDTO(15-20)src/main/webapp/app/openapi/model/codeGenerationResultDTO.ts (1)
CodeGenerationResultDTO(15-28)src/main/webapp/app/programming/shared/entities/programming-exercise.model.ts (1)
ProgrammingExercise(47-96)
src/test/java/de/tum/cit/aet/artemis/hyperion/CodeGenerationResultDTOTest.java (1)
src/main/webapp/app/openapi/model/codeGenerationResultDTO.ts (1)
CodeGenerationResultDTO(15-28)
src/test/java/de/tum/cit/aet/artemis/hyperion/CodeGenerationRequestDTOTest.java (1)
src/main/webapp/app/openapi/model/codeGenerationRequestDTO.ts (1)
CodeGenerationRequestDTO(15-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: server-style
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
🔇 Additional comments (9)
src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationResultDTO.java (1)
65-72: Validate or document negative attempt countsThe DTO currently permits negative
attempts(seetestCodeGenerationResultDTO_WithNegativeAttempts), yet production code only uses ≥0 values. Add validation to reject negatives, document why negatives are allowed, or remove these tests if invalid.src/test/java/de/tum/cit/aet/artemis/hyperion/CodeGenerationResultDTOTest.java (3)
113-134: LGTM! JSON inclusion behavior is properly tested.The tests correctly verify that the
JsonInclude.NON_EMPTYannotation behavior works as expected: non-empty fields are included (lines 113-122) and empty strings are excluded (lines 125-134). If the DTO class lacks this annotation, the test at lines 125-134 will fail, making this an effective regression guard.
137-149: LGTM! Equality and hashCode contract is validated.The test properly verifies that equal objects have equal hash codes, and unequal objects have different hash codes, satisfying the equals/hashCode contract.
192-224: LGTM! Common scenario coverage is thorough.The tests cover realistic success and failure messages for all three repository types (solution, template, test), ensuring the DTO handles production use cases correctly.
src/test/java/de/tum/cit/aet/artemis/hyperion/web/HyperionCodeGenerationResourceTest.java (3)
61-80: LGTM! Public API test follows best practices.This test properly validates the happy path through the public
generateCode()API:
- Mocks are set up correctly
- Assertions verify response status, body content, and service interaction
- Test is focused and easy to understand
82-99: LGTM! Failure scenarios are well covered.These tests properly validate error handling through the public API, covering both failed results and null results (which represent maximum retry failures). The repository-type-specific failure messages are correctly verified.
Also applies to: 101-116
367-398: LGTM! Exception handling and exam exercise tests are solid.These tests verify:
- Proper error handling when the execution service throws exceptions (lines 367-380)
- Correct processing of exam exercises (lines 382-398)
Both scenarios are important edge cases that are well covered.
src/test/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationRequestDTO.java (2)
19-19: Verify filename matches class name.The file path metadata indicates
CodeGenerationRequestDTO.java, but the class name isCodeGenerationRequestDTOTest. In Java, the filename must match the public class name. If this is a metadata error in the review system, no action is needed. If the file is actually misnamed in the repository, it must be renamed toCodeGenerationRequestDTOTest.javato compile.
117-125: No change needed:toString()returns lowercase
RepositoryType.toString()is overridden to return the lowercasenamefield, so the test’sassertThat(toString).contains("solution")is correct.
src/test/java/de/tum/cit/aet/artemis/hyperion/CodeGenerationRequestDTOTest.java
Outdated
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
luis-gasparschroeder
dismissed stale reviews from HawKhiem and coderabbitai[bot]
via
0f10c11
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java (1)
309-309: Remove unnecessary exception for IrisTutorSuggestionIntegrationTest.
The test class doesn’t import or useRestController, so it shouldn’t be whitelisted intestNoRestControllersImported. Remove it from the exception list.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: server-tests
- GitHub Check: client-tests
- GitHub Check: Analyse
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
5 participants
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
Beforehand, instructors had to implement the solution, template, and test repository themselves. This was error-prone and time-intensive, as instructors have many other relevant tasks besides exercise creation. The code generation supports their workflow by suggesting and concrete and tested repository structured, aligned with the programming exercise.
Description
This PR introduces a comprehensive code generation system that automatically creates solution code, student templates, and test suites for programming exercises.
Key Components:
Technical Implementation:
Steps for Testing
Review Progress
Performance Review
The code generation takes between 1 and 5 minutes, depending on the number of re-iterations. The reason is the build latency and multi-stages LLM pipeline.
Using the OpenAI GPT-5 LLM, the code generation costs approximately $ 0.05 (assuming three reiterations and an already written problem statement).
Code Review
Manual Tests
Performance Tests
Follow-Up PRs
The three follow-ups can utilize the existing infrastructure and should be within a reasonable workload (1-3 days).
Test Coverage
Server Coverage Report
Screenshots
Hyperion_Code_Generation_with_Artemis.mp4
Summary by CodeRabbit
New Features
Improvements
API/Client
Documentation