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.
I'm seeing the "Create Assistance" button even though I set hyperion: false
Please also update your PR description. Try to also fill the test coverage report, but it seems that the test checks need to pass for that (which is sometimes flaky). I mentioned on Monday that a discussion / issues section in the PR description would be needed so we can start to track follow-up issues and we know what we are getting into transparently when we merge this PR.
Considerations include:
- File deletion
- Starting with a minimal repository state for generation
- Default tests case don't make sense -> We'd need to start blank?
- Observability / cost tracking
- Double invocation guard (currently you can just press "Generate code" again once you refresh, it also does not reject it)
- UI is not updating except for the build results
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.ts (1)
32-46: Remove duplicate imports.
ArtemisTranslatePipeappears on lines 32 and 46, andNgbTooltipappears on lines 41 and 45. Remove the duplicates to keep the imports clean.Apply this diff:
imports: [ FaIconComponent, TranslateDirective, ArtemisTranslatePipe, CodeEditorContainerComponent, IncludedInScoreBadgeComponent, ProgrammingExerciseInstructorExerciseStatusComponent, NgbDropdown, NgbDropdownToggle, NgbDropdownMenu, NgbDropdownButtonItem, NgbDropdownItem, NgbTooltip, UpdatingResultComponent, ProgrammingExerciseStudentTriggerBuildButtonComponent, ProgrammingExerciseEditableInstructionComponent, - NgbTooltip, - ArtemisTranslatePipe, ],
♻️ Duplicate comments (3)
src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.html (3)
135-138: Add an accessible name for the spinner-only button.The spinner button lacks an accessible name for assistive technologies. Add a visually hidden translated label or
aria-label.Apply this diff to add a hidden accessible label:
@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>
136-138: [Duplicate] Add accessible label for spinner button.This spinner-only button lacks an accessible name, making it inaccessible to assistive technologies. Previous review comments have already flagged this issue and provided a solution.
150-152: [Duplicate] Missing i18n translation on hardcoded text.The button contains hardcoded English text "Generate Code" despite having the
jhiTranslatedirective. The translation key should provide the text content, not duplicate it. Previous reviewers have already flagged missing i18n throughout this section.
🧹 Nitpick comments (4)
src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.ts (4)
75-122: Consider usingfinalize()to guarantee state reset.The
isGeneratingCodesignal is reset in both thenextanderrorcallbacks, but if the subscription completes or errors in an unexpected way, the state might not be reset. Using thefinalize()operator ensures the flag is always reset regardless of how the observable completes.Apply this diff to add
finalize():+import { finalize } from 'rxjs'; + generateCode(): void { if (!this.exercise?.id || this.isGeneratingCode()) { return; } // Only allow code generation for supported repository types if (this.selectedRepository !== RepositoryType.TEMPLATE && this.selectedRepository !== RepositoryType.SOLUTION && this.selectedRepository !== RepositoryType.TESTS) { this.codeGenAlertService.addAlert({ type: AlertType.WARNING, translationKey: 'artemisApp.programmingExercise.codeGeneration.unsupportedRepository', }); return; } this.isGeneratingCode.set(true); const request: CodeGenerationRequestDTO = { repositoryType: this.selectedRepository as CodeGenerationRequestDTO.RepositoryTypeEnum, }; - this.codeGenerationService.generateCode(this.exercise.id, request).subscribe({ + this.codeGenerationService.generateCode(this.exercise.id, request) + .pipe(finalize(() => this.isGeneratingCode.set(false))) + .subscribe({ next: (response) => { - this.isGeneratingCode.set(false); - if (response.success) { this.codeGenAlertService.addAlert({ type: AlertType.SUCCESS, translationKey: 'artemisApp.programmingExercise.codeGeneration.success', translationParams: { repositoryType: this.selectedRepository }, }); } else { this.codeGenAlertService.addAlert({ type: AlertType.WARNING, translationKey: 'artemisApp.programmingExercise.codeGeneration.partialSuccess', translationParams: { repositoryType: this.selectedRepository }, }); } }, error: () => { - this.isGeneratingCode.set(false); this.codeGenAlertService.addAlert({ type: AlertType.DANGER, translationKey: 'artemisApp.programmingExercise.codeGeneration.error', translationParams: { repositoryType: this.selectedRepository }, }); }, }); }
89-94: Consider safer type handling for repository enum.The type assertion on line 92 assumes
this.selectedRepositorywill always match theCodeGenerationRequestDTO.RepositoryTypeEnumafter the validation check. While the validation on lines 81-87 ensures only TEMPLATE, SOLUTION, or TESTS are allowed, the string values must exactly match the enum.Consider adding explicit mapping or runtime validation to make the contract more explicit:
const request: CodeGenerationRequestDTO = { - repositoryType: this.selectedRepository as CodeGenerationRequestDTO.RepositoryTypeEnum, + repositoryType: this.selectedRepository as 'TEMPLATE' | 'SOLUTION' | 'TESTS', };Or verify the enum values match at the validation site to make the relationship explicit.
95-121: Add explicit subscription cleanup or use async patterns.The observable subscription created on line 95 is not explicitly unsubscribed. While HTTP observables complete automatically after a single emission, consider using declarative patterns for consistency and to avoid potential memory leaks if the subscription is modified in the future.
Consider one of these approaches:
Option 1: Store subscription and clean up in ngOnDestroy
private subscription = new Subscription(); ngOnDestroy() { this.subscription.unsubscribe(); } // In generateCode(): this.subscription.add( this.codeGenerationService.generateCode(this.exercise.id, request).subscribe({ // handlers... }) );Option 2: Use takeUntilDestroyed operator (Angular 16+)
import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; // In generateCode(): this.codeGenerationService .generateCode(this.exercise.id, request) .pipe(takeUntilDestroyed(this.destroyRef)) .subscribe({ // handlers... });
95-121: Consider providing more actionable error feedback.The error handler on lines 113-120 catches all errors and shows a generic danger alert. For better user experience and debugging, consider:
- Logging the error details for troubleshooting
- Distinguishing between different error types (network, validation, server errors)
- Providing more specific guidance based on the error type
Example enhancement:
error: (error) => { this.isGeneratingCode.set(false); + console.error('Code generation failed:', error); + + const translationKey = error.status === 400 + ? 'artemisApp.programmingExercise.codeGeneration.validationError' + : 'artemisApp.programmingExercise.codeGeneration.error'; + this.codeGenAlertService.addAlert({ type: AlertType.DANGER, - translationKey: 'artemisApp.programmingExercise.codeGeneration.error', + translationKey, translationParams: { repositoryType: this.selectedRepository }, }); },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
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)
🧰 Additional context used
📓 Path-based instructions (2)
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
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
🧬 Code graph analysis (1)
src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.ts (2)
src/main/webapp/app/app.constants.ts (1)
MODULE_FEATURE_HYPERION(40-40)src/main/webapp/app/openapi/model/codeGenerationRequestDTO.ts (3)
CodeGenerationRequestDTO(15-20)RepositoryTypeEnum(22-26)RepositoryTypeEnum(27-27)
⏰ 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). (8)
- 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: server-style
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
🔇 Additional comments (9)
src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.html (4)
132-157: Adjust UI placement per tester feedback.According to the PR objectives, FelixTJDietrich reported that the "Creation Assistance" dropdown should be placed to the right of the "Create Assignment Repository" button in the same button group (not as a separate
btn-group), and the result/feedback progress should appear to the right of that button as previously designed.Currently, the Hyperion code generation UI is a standalone
btn-groupthat appears after the delete button. Please verify the intended layout with the design and adjust the button placement accordingly to match the expected UX flow.
174-174: LGTM—single-line refactor preserves all bindings.The trigger-build button has been condensed to a single line while preserving all attributes (
[exercise],[participation],[btnSize]). This improves readability without changing functionality.
132-157: Verify UI placement aligns with PR feedback.According to the PR objectives, FelixTJDietrich reported that the "Creation Assistance" dropdown should be placed to the right of the "Create Assignment Repository" button in the same button group, with results/feedback appearing to the right of that button. The current implementation places the Hyperion controls in a separate
btn-groupdiv outside the repository creation/deletion button context. Please verify this matches the intended UX design and tester feedback.Based on the PR objectives feedback, you may need to adjust the layout so the AI generation controls are visually grouped with the repository action buttons rather than appearing as a separate control cluster.
174-174: LGTM: Multi-line component simplified correctly.The simplification from multi-line to single-line format maintains all required bindings (
[exercise],[participation],[btnSize]) and improves readability.src/main/webapp/app/programming/manage/code-editor/instructor-and-editor-container/code-editor-instructor-and-editor-container.component.ts (5)
68-68: Good use of signal API for reactive state.Using
signal(false)forisGeneratingCodealigns with Angular's modern signal-based reactivity and prepares the component for zoneless rendering. This addresses the earlier review feedback.
65-70: Approve service injection and feature flag integration.The use of
inject()for dependency injection, the Hyperion feature flag check viaProfileService, and the signal-based state management are all clean and idiomatic. The integration withHyperionCodeGenerationApiServicefollows Angular best practices.
17-23: LGTM: Clean dependency imports.The imports are well-organized and include all necessary dependencies for the Hyperion code generation feature. The use of OpenAPI-generated types ensures type safety with the backend contract.
65-70: LGTM: Modern Angular patterns with signal state.Good use of the
inject()function for dependency injection and the signal API for reactive state management. This addresses the previous review feedback and aligns with Angular's direction toward zoneless change detection.
76-87: LGTM: Robust validation guards.The validation logic correctly:
- Guards against missing exercise ID or concurrent generation
- Restricts to supported repository types (TEMPLATE, SOLUTION, TESTS)
- Provides user feedback for unsupported repositories
The repository type check aligns with the DTO enum definition.
...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.
Tested locally and it works, we also need to test on staging before merging.
The "Create Assistance" button is also correctly removed when hyperion is disabled.
Issues to be aware of / follow-ups:
- The current approach does not clean up / delete (existing) files
- Idea: Start with a minimal repository state for generation
- Default tests case don't make sense -> We'd need to start blank?
- It is a bit intransparent what is going on in the code generation pipeline from the logs alone, we need some better observability and also cost tracking in the future
- We do not guard agains double invocation (currently you can just press "Generate code" again once you refresh, it also does not reject it)
- UI is not updating except for the build results so we have to refresh after the code generation does a commit to see the changed files
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
- Update HyperionCodeGenerationExecutionServiceTest to use new constructor signature - Add missing HyperionProgrammingExerciseContextRendererService parameter to template and test repository service tests - Remove dependency on ApplicationContext in favor of direct strategy injection - Fix compilation errors that were preventing server test coverage generation
…s1intum/Artemis into feature/hyperion/code-generation-clean
luis-gasparschroeder
dismissed stale reviews from HawKhiem and FelixTJDietrich
via
620a048
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.java (1)
138-140: Test name claims "callsGitService" but no interaction is verified; also repository is null.The test name suggests that GitService interactions are being verified, but:
gitService.getOrCheckoutRepository(null, false, "main", false)returns null on an unmocked call- No
verify()statements assert that GitService methods were invoked- The test only ensures no exception is thrown
Options to fix (per previous review):
- Rename to reflect actual behavior:
cleanupRepository_withRepositoryAndCommitHash_doesNotThrow- Or stub a mock Repository and verify expected GitService interactions inside
cleanupRepositoryMinimal rename:
- void cleanupRepository_withValidInputs_callsGitService() throws Exception { + void cleanupRepository_withRepositoryAndCommitHash_doesNotThrow() throws Exception { ReflectionTestUtils.invokeMethod(service, "cleanupRepository", gitService.getOrCheckoutRepository(null, false, "main", false), "commit-hash"); }src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryTest.java (1)
158-165: Repository-access exception test still expectsRuntimeException.We still stub the chat model to throw a generic
RuntimeExceptionand assert the same, even though the production code converts repository/network failures intoNetworkingExceptionvia the context renderer path. Please trigger the failure throughcontextRenderer.getExistingSolutionCode(...)(or the corresponding collaborator) and assert that aNetworkingExceptionis emitted, as flagged in the earlier review.Apply this diff to align the set-up and expectation:
- when(chatModel.call(any(Prompt.class))).thenThrow(new RuntimeException("Repository access failed")); - - assertThatThrownBy(() -> templateRepository.generateSolutionPlan(user, exercise, "logs", "structure")).isInstanceOf(RuntimeException.class) - .hasMessageContaining("Repository access failed"); + when(contextRenderer.getExistingSolutionCode(eq(exercise), eq(gitService))) + .thenThrow(new NetworkingException("Repository access failed")); + + assertThatThrownBy(() -> templateRepository.generateSolutionPlan(user, exercise, "logs", "structure")).isInstanceOf(NetworkingException.class) + .hasMessageContaining("Repository access failed");
🧹 Nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.java (2)
62-63: Remove unused mock.The
mockStrategymock is declared but never used in any test. Remove it to keep the test class clean.- @Mock - private HyperionCodeGenerationService mockStrategy; - private HyperionCodeGenerationExecutionService service;
27-151: Consider adding tests for the main generation orchestration flow.The current tests cover strategy resolution, null handling, and delegation, but don't test the core orchestration logic in
generateAndCompileCodewith valid inputs. Consider adding tests for:
- Valid repository URI with successful generation
- Iteration/retry logic with CI feedback
- Strategy invocation and response handling
- Build log extraction with actual Result objects
These may be better suited as integration tests if they require complex setup or external dependencies.
📜 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/codegeneration/HyperionCodeGenerationExecutionServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTestRepositoryTest.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/HyperionTestRepositoryTest.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/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryTest.java
🧬 Code graph analysis (1)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryTest.java (2)
src/main/java/de/tum/cit/aet/artemis/programming/domain/Repository.java (1)
Repository(19-102)src/main/java/de/tum/cit/aet/artemis/programming/domain/VcsRepositoryUri.java (1)
VcsRepositoryUri(11-180)
⏰ 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: 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: server-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: bean-instantiation-check
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (6)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.java (6)
71-85: LGTM! Constructor issue from past review is resolved.The setup correctly instantiates
HyperionCodeGenerationExecutionServicewith the three strategy service mocks (solutionStrategy, templateStrategy, testStrategy) instead of the deprecated ApplicationContext approach. The test fixtures are clean and follow the coding guidelines.
87-112: LGTM! Strategy resolution tests are well-structured.The four tests comprehensively cover strategy resolution for supported repository types (SOLUTION, TEMPLATE, TESTS) and unsupported types (AUXILIARY). Each test is focused, uses descriptive naming, and properly verifies the injected strategy mocks.
114-121: LGTM! Delegation to GitService is properly verified.The test correctly stubs
gitService.getDefaultBranch()and verifies the private method returns the expected value.
123-128: LGTM! Null repository URI handling is verified.The test correctly validates that
generateAndCompileCodereturns null when the exercise has no repository URI configured.
130-135: LGTM! Null result handling is verified.The test properly validates the default error message returned when the build result is null.
143-150: LGTM! Null-input handling tests are appropriately named and focused.These two tests correctly verify that
cleanupRepositoryhandles null repository and null commit hash without throwing exceptions. The test names accurately reflect their intent.
...a/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryTest.java
Show resolved
Hide resolved
...a/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationService.java (1)
45-49: Make ChatClient mandatory or fail fast
chatClientis injected withrequired = false, yet every subclass callscallChatClient(...)unconditionally. When Hyperion is enabled but Spring AI is misconfigured or missing, this becomes a guaranteedNullPointerExceptionduring generation. Either require a bean (remove the optional injection andObjects.requireNonNullit) or throw a descriptiveNetworkingExceptionup front when it is absent. This was raised earlier and remains unresolved.src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.java (1)
138-140: Test still never exercises cleanupRepository
gitService.getOrCheckoutRepository(...)on a mock returns null, so you end up callingcleanupRepository(null, "commit-hash"). The method exits without touchinggitService.resetToOriginHead, yet the test name claims it verifies that call. Please stub a realRepository(or just pass a mock) and assert thatresetToOriginHeadis invoked, or rename the test to match the no-op behavior. This was flagged earlier but remains unresolved.src/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionService.java (1)
402-405: Don’t swallowInterruptedExceptionwhile polling CIThe
catch (Exception e)block also absorbsInterruptedException, clears the interrupt flag, and even callsThread.sleepagain. That breaks shutdown and cancellation semantics. Handle interrupts explicitly and rethrow:- catch (Exception e) { - log.warn("Exception while polling for build result for commit {}: {}. Continuing...", commitHash, e.getMessage()); - Thread.sleep(POLL_INTERVAL); - } + catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + throw ie; + } + catch (Exception e) { + log.warn("Exception while polling for build result for commit {}: {}. Continuing...", commitHash, e.getMessage()); + } + + Thread.sleep(POLL_INTERVAL);Also move the final
Thread.sleepoutside the try/catch so we still respect the poll interval on the happy path.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/main/java/de/tum/cit/aet/artemis/hyperion/dto/ArtifactLocationDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/dto/ArtifactTypeDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationResponseDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/dto/ConsistencyIssueCategoryDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/dto/ConsistencyIssueDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/dto/GeneratedFileDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/dto/SeverityDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionConsistencyCheckService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionConsistencyCheckServiceTest.java(2 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/service/codegeneration/HyperionCodeGenerationStrategyTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionSolutionRepsitoryTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTestRepositoryTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryTest.java
- src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTestRepositoryTest.java
🧰 Additional context used
📓 Path-based instructions (2)
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/hyperion/dto/ArtifactTypeDTO.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/dto/ArtifactLocationDTO.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/dto/GeneratedFileDTO.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/dto/ConsistencyIssueDTO.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/dto/SeverityDTO.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionService.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/dto/ConsistencyIssueCategoryDTO.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionConsistencyCheckService.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationService.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/dto/CodeGenerationResponseDTO.java
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/HyperionConsistencyCheckServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionSolutionRepsitoryTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionCodeStyleArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationStrategyTest.java
🧬 Code graph analysis (3)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionService.java (4)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationService.java (1)
Service(32-157)src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionProgrammingExerciseContextRendererService.java (1)
Service(93-371)src/main/java/de/tum/cit/aet/artemis/programming/domain/Repository.java (1)
Repository(19-102)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (1)
LocalVCRepositoryUri(17-318)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionConsistencyCheckService.java (2)
src/main/webapp/app/openapi/model/consistencyIssue.ts (1)
ConsistencyIssue(16-37)src/main/webapp/app/openapi/model/artifactLocation.ts (1)
ArtifactLocation(15-32)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationService.java (5)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionService.java (1)
Service(46-410)src/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryService.java (1)
Service(28-94)src/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTestRepositoryService.java (1)
Service(28-94)src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionProgrammingExerciseContextRendererService.java (1)
Service(93-371)src/main/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionSolutionRepositoryService.java (1)
Service(24-69)
⏰ 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: 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: Build .war artifact
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
- GitHub Check: Mend Security Check
.../cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionService.java
Show resolved
Hide resolved
.../cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationExecutionService.java
Show resolved
Hide resolved
...va/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationService.java
Show resolved
Hide resolved
.../tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationStrategyTest.java
Outdated
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: 1
♻️ Duplicate comments (8)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryTest.java (3)
108-120: Still not asserting solution-code propagationThis test keeps passing even if the solution snippet never reaches the prompt variables—exactly the gap we flagged earlier. Please capture the
Mappassed torenderObject(...)and assert it contains the sentinel returned bycontextRenderer.getExistingSolutionCode(...).
122-185: Failure-path tests remain false positivesAll four scenario-specific tests still execute the happy path: no checkout failure, no multi-file concatenation, no IO exception, no empty result. They will never catch regressions in those branches. Please stub the relevant collaborators to reproduce each condition (or drop redundant tests) and assert the expected warnings/errors.
163-171: Wrong collaborator mocked for repository-access failure
generateSolutionPlan_withRepositoryAccessException_throwsNetworkingExceptionstill stubschatModel.call(...)to throwRuntimeException, so we are not exercising repository access at all. Instead, makecontextRenderer.getExistingSolutionCode(...)throw theNetworkingExceptionyou expect and assert that propagation. This aligns the test with the production failure path.src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTestRepositoryTest.java (5)
95-107: Test name implies verifying template variables; assertion currently doesn't check them.This test only verifies that
render()was invoked. Per previous review feedback, capture the variables and assert expected keys/values (e.g.,language,repositoryStructure,buildLogs,solutionCode).Based on past review comments.
109-120: Test name is misleading; no failure is simulated and no warning is asserted.The test name claims
withFailedRepositoryCheckout_usesWarningMessage, but all mocks return successful responses and no warning message is asserted. Either simulate a repository checkout failure (e.g., stubcontextRenderer.getExistingSolutionCodeto throw an exception or return an error indicator) and assert the warning message in template variables, or rename the test to reflect current behavior.Based on past review comments.
122-134: Test name is misleading; no multi-file input is provided and no concatenation is asserted.The test name claims
withMultipleJavaFiles_concatenatesContent, but the mock returns a single solution code and no concatenation is verified. Either stubcontextRenderer.getExistingSolutionCodeto return multiple concatenated files and assert the concatenated content in template variables, or rename the test to reflect current behavior.Based on past review comments.
136-148: Test name is misleading; no IOException is induced and no error message is asserted.The test name claims
withIOExceptionDuringWalk_returnsErrorMessage, but all mocks return successful responses and no error message is asserted. Either stubcontextRenderer.getExistingSolutionCodeto throw anIOExceptionand assert the error message in the template variables or result, or rename the test to reflect current behavior.Based on past review comments.
160-172: Test name is misleading; no "no files" condition is simulated and no message is asserted.The test name claims
withNoJavaFiles_returnsNoFilesMessage, but the mock returns"public class Solution {}"(indicating a Java file is present) and no "no files" message is asserted. Either stubcontextRenderer.getExistingSolutionCodeto return an empty string or specific message and assert that message in the template variables or result, or rename the test to reflect current behavior.Based on past review comments.
🧹 Nitpick comments (5)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationStrategyTest.java (1)
86-92: Assert the intended invocation order explicitlyThe comment claims we verify the four steps execute “in correct order”, yet
times(4)only checks call count. If the production pipeline ever shuffled the sequence, this test would stay green. Please switch to anInOrderverification (plan → structure → headers → logic) so the test fails when the orchestration regresses.src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionSolutionRepsitoryTest.java (1)
35-35: Rename the file to match the class.The file is named
HyperionSolutionRepsitoryTest.javawhile the class isHyperionSolutionRepositoryServiceTest. Please rename the file to match the class to avoid confusion during navigation and future maintenance.src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTestRepositoryTest.java (3)
77-93: Consider capturing and asserting template variables.While this test validates the happy path, it doesn't capture the
Mappassed totemplates.renderObjectto verify the template variables contain expected keys like"language","repositoryStructure","buildLogs", and"solutionCode". This would strengthen confidence that the correct context is passed to the template.Example:
var captor = ArgumentCaptor.forClass(Map.class); verify(templates).renderObject(eq("/prompts/hyperion/test/1_plan.st"), captor.capture()); Map<String, Object> vars = captor.getValue(); assertThat(vars).containsKeys("language", "repositoryStructure", "buildLogs", "solutionCode");
241-253: Consider capturing template variables to verify empty string usage.While this test validates that null
buildLogsdoesn't cause an exception, it doesn't capture the template variables to confirm that an empty string is actually passed to the template. Consider using anArgumentCaptorto verify the"buildLogs"key in the template variables contains an empty string.Example:
var captor = ArgumentCaptor.forClass(Map.class); verify(templates).renderObject(eq("/prompts/hyperion/test/1_plan.st"), captor.capture()); Map<String, Object> vars = captor.getValue(); assertThat(vars).containsEntry("buildLogs", "");
255-267: Consider capturing template variables to verify empty string usage.While this test validates that null
repositoryStructuredoesn't cause an exception, it doesn't capture the template variables to confirm that an empty string is actually passed to the template. Consider using anArgumentCaptorto verify the"repositoryStructure"key in the template variables contains an empty string.Example:
var captor = ArgumentCaptor.forClass(Map.class); verify(templates).renderObject(eq("/prompts/hyperion/test/1_plan.st"), captor.capture()); Map<String, Object> vars = captor.getValue(); assertThat(vars).containsEntry("repositoryStructure", "");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationStrategyTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionSolutionRepsitoryTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTestRepositoryTest.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/codegeneration/HyperionTemplateRepositoryTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionCodeGenerationStrategyTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTestRepositoryTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionSolutionRepsitoryTest.java
🧬 Code graph analysis (1)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTemplateRepositoryTest.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/domain/VcsRepositoryUri.java (1)
VcsRepositoryUri(11-180)
⏰ 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). (12)
- GitHub Check: client-tests
- GitHub Check: client-compilation
- GitHub Check: client-tests-selected
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- 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: bean-instantiation-check
- GitHub Check: Analyse
🔇 Additional comments (7)
src/test/java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTestRepositoryTest.java (7)
62-75: LGTM: Test setup is complete and correct.The setup method properly initializes all mocks and dependencies, including the
contextRendererthat was previously missing. The test fixtures (user and exercise) are appropriately minimal for unit testing.
174-188: LGTM: Test validates file structure generation correctly.The test properly verifies that
defineFileStructurereturns the expected file structure with correct path and content. Assertions are specific and useassertThatas per guidelines.
190-204: LGTM: Test validates multi-step prompting correctly.The test properly verifies that
generateClassAndMethodHeaderscalls both file structure and headers generation, and asserts the expected method signature in the result. Interaction verification ensures both prompts are invoked.
206-222: LGTM: Test validates full generation pipeline correctly.The test properly verifies that
generateCoreLogicorchestrates all three steps (file structure, headers, logic) and asserts the expected test implementation in the result. Interaction verification ensures all prompts are invoked in sequence.
224-229: LGTM: Test validates repository type correctly.Simple and clear test that verifies the service returns the correct repository type.
231-239: LGTM: Test validates exception mapping correctly.The test properly verifies that
NonTransientAiExceptionfrom the AI service is mapped toNetworkingExceptionwith an appropriate error message.
269-273: LGTM: Helper method improves test readability.This private helper encapsulates
ChatResponsecreation, making test setup cleaner and more maintainable.
.../java/de/tum/cit/aet/artemis/hyperion/service/codegeneration/HyperionTestRepositoryTest.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
🧹 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.
| class HyperionPromptTemplateServiceTest { | ||
|
|
||
| private HyperionPromptTemplateService templateService; | ||
|
|
||
| @BeforeEach | ||
| void setup() { | ||
| templateService = new HyperionPromptTemplateService(); | ||
| } | ||
|
|
||
| @Test | ||
| void render_withValidTemplateAndVariables_replacesPlaceholders() throws Exception { | ||
| String templateContent = "Hello {{name}}, your score is {{score}}!"; | ||
|
|
||
| Map<String, String> variables = Map.of("name", "John", "score", "95"); | ||
|
|
||
| Map<String, Object> objectVariables = Map.of("name", "John", "score", 95); | ||
|
|
||
| assertThatThrownBy(() -> templateService.renderObject("/nonexistent/template.st", objectVariables)).isInstanceOf(RuntimeException.class) | ||
| .hasMessageContaining("Failed to load template"); | ||
| } | ||
|
|
||
| @Test | ||
| void render_withStringVariables_replacesCorrectly() { | ||
| String template = "Programming language: {{language}}, Problem: {{problem}}"; | ||
| Map<String, String> variables = Map.of("language", "Java", "problem", "Implement sorting"); | ||
|
|
||
| assertThatThrownBy(() -> templateService.render("/nonexistent/template.st", variables)).isInstanceOf(RuntimeException.class) | ||
| .hasMessageContaining("Failed to load template"); | ||
| } | ||
|
|
||
| @Test | ||
| void renderObject_withObjectVariables_handlesConversion() { | ||
| Map<String, Object> variables = Map.of("exerciseId", 123L, "language", "JAVA", "enabled", true); | ||
|
|
||
| assertThatThrownBy(() -> templateService.renderObject("/nonexistent/template.st", variables)).isInstanceOf(RuntimeException.class) | ||
| .hasMessageContaining("Failed to load template"); | ||
| } | ||
|
|
||
| @Test | ||
| void render_withNullResourcePath_throwsException() { | ||
| Map<String, String> variables = Map.of("key", "value"); | ||
|
|
||
| assertThatThrownBy(() -> templateService.render(null, variables)).isInstanceOf(RuntimeException.class); | ||
| } | ||
|
|
||
| @Test | ||
| void renderObject_withNullResourcePath_throwsException() { | ||
| Map<String, Object> variables = Map.of("key", "value"); | ||
|
|
||
| assertThatThrownBy(() -> templateService.renderObject(null, variables)).isInstanceOf(RuntimeException.class); | ||
| } | ||
|
|
||
| @Test | ||
| void render_withEmptyVariables_processesTemplate() { | ||
| Map<String, String> variables = Map.of(); | ||
|
|
||
| assertThatThrownBy(() -> templateService.render("/nonexistent/template.st", variables)).isInstanceOf(RuntimeException.class) | ||
| .hasMessageContaining("Failed to load template"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add positive test cases for successful template rendering.
The test class currently only validates error scenarios (nonexistent templates, null inputs, empty variables). Critical functionality—successful template rendering and correct variable substitution—is not tested. This leaves core behavior unvalidated and could allow bugs in the primary use case to go undetected.
Add test cases that:
- Load an actual template resource from
src/test/resources/(e.g.,src/test/resources/prompts/test-template.st) - Verify successful variable substitution for both
render()andrenderObject() - Validate edge cases such as:
- Templates with multiple placeholders
- Missing variables (what's the expected behavior?)
- Special characters in variable values
- Numeric and boolean conversion in
renderObject()
Example test structure:
@Test
void render_withValidTemplate_replacesPlaceholders() throws Exception {
// Arrange: create test template in src/test/resources/prompts/test-template.st
// Content: "Hello {{name}}, your score is {{score}}!"
Map<String, String> variables = Map.of("name", "John", "score", "95");
// Act
String result = templateService.render("/prompts/test-template.st", variables);
// Assert
assertThat(result).isEqualTo("Hello John, your score is 95!");
}
@Test
void renderObject_withMixedTypes_convertsAndReplacesCorrectly() throws Exception {
// Test numeric and boolean conversion
Map<String, Object> variables = Map.of("id", 123L, "enabled", true, "name", "Test");
String result = templateService.renderObject("/prompts/test-template-objects.st", variables);
assertThat(result).contains("123", "true", "Test");
}🤖 Prompt for AI Agents
In
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java
around lines 10 to 69, add positive unit tests that load real template files
from src/test/resources/prompts (e.g. create test-template.st and
test-template-objects.st) and assert successful rendering: (1) a test that calls
templateService.render("/prompts/test-template.st",
Map.of("name","John","score","95")) and asserts the exact substituted output
"Hello John, your score is 95!"; (2) a test that calls
templateService.renderObject("/prompts/test-template-objects.st",
Map.of("id",123L,"enabled",true,"name","Test")) and asserts the numeric and
boolean values are converted and present ("123", "true", "Test"); also add short
tests for multiple placeholders, special characters in variable values, and one
asserting the service's documented behavior for missing variables (either
preserved placeholder or replaced with empty string) so the expected behavior is
explicitly asserted.
| @Test | ||
| void render_withValidTemplateAndVariables_replacesPlaceholders() throws Exception { | ||
| String templateContent = "Hello {{name}}, your score is {{score}}!"; | ||
|
|
||
| Map<String, String> variables = Map.of("name", "John", "score", "95"); | ||
|
|
||
| Map<String, Object> objectVariables = Map.of("name", "John", "score", 95); | ||
|
|
||
| assertThatThrownBy(() -> templateService.renderObject("/nonexistent/template.st", objectVariables)).isInstanceOf(RuntimeException.class) | ||
| .hasMessageContaining("Failed to load template"); | ||
| } |
There was a problem hiding this comment.
Remove unused variables and align test with its name.
The test method name render_withValidTemplateAndVariables_replacesPlaceholders suggests a positive test case for successful template rendering, but the implementation only tests the failure path with a nonexistent template. Additionally, lines 21-25 declare templateContent, variables, and objectVariables that are never used in the test.
Either:
- Remove the unused variables and rename the test to reflect its actual behavior (e.g.,
renderObject_withNonexistentTemplate_throwsException), OR - Add a positive test case that validates successful template rendering with the declared variables.
Apply this diff to remove unused variables and rename appropriately:
@Test
- void render_withValidTemplateAndVariables_replacesPlaceholders() throws Exception {
- String templateContent = "Hello {{name}}, your score is {{score}}!";
-
- Map<String, String> variables = Map.of("name", "John", "score", "95");
-
+ void renderObject_withNonexistentTemplate_throwsException() {
Map<String, Object> objectVariables = Map.of("name", "John", "score", 95);
assertThatThrownBy(() -> templateService.renderObject("/nonexistent/template.st", objectVariables)).isInstanceOf(RuntimeException.class)Alternatively, add a separate positive test case using an actual template resource.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| void render_withValidTemplateAndVariables_replacesPlaceholders() throws Exception { | |
| String templateContent = "Hello {{name}}, your score is {{score}}!"; | |
| Map<String, String> variables = Map.of("name", "John", "score", "95"); | |
| Map<String, Object> objectVariables = Map.of("name", "John", "score", 95); | |
| assertThatThrownBy(() -> templateService.renderObject("/nonexistent/template.st", objectVariables)).isInstanceOf(RuntimeException.class) | |
| .hasMessageContaining("Failed to load template"); | |
| } | |
| @Test | |
| void renderObject_withNonexistentTemplate_throwsException() { | |
| Map<String, Object> objectVariables = Map.of("name", "John", "score", 95); | |
| assertThatThrownBy(() -> templateService.renderObject("/nonexistent/template.st", objectVariables)) | |
| .isInstanceOf(RuntimeException.class) | |
| .hasMessageContaining("Failed to load template"); | |
| } |
🤖 Prompt for AI Agents
In
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java
around lines 19-29, remove the unused local variables templateContent,
variables, and objectVariables, and rename the test method from
render_withValidTemplateAndVariables_replacesPlaceholders to
renderObject_withNonexistentTemplate_throwsException so the method name matches
the asserted behavior (assertThatThrownBy(...) checking for "Failed to load
template"); keep the existing assertion and exception expectation unchanged.
| @Test | ||
| void render_withStringVariables_replacesCorrectly() { | ||
| String template = "Programming language: {{language}}, Problem: {{problem}}"; | ||
| Map<String, String> variables = Map.of("language", "Java", "problem", "Implement sorting"); | ||
|
|
||
| assertThatThrownBy(() -> templateService.render("/nonexistent/template.st", variables)).isInstanceOf(RuntimeException.class) | ||
| .hasMessageContaining("Failed to load template"); | ||
| } |
There was a problem hiding this comment.
Remove unused variables and align test with its name.
Similar to the previous test, the method name render_withStringVariables_replacesCorrectly suggests a positive scenario, but the implementation only tests failure. Lines 33-34 declare template and variables that are never used.
Apply this diff to remove unused variables and rename appropriately:
@Test
- void render_withStringVariables_replacesCorrectly() {
- String template = "Programming language: {{language}}, Problem: {{problem}}";
- Map<String, String> variables = Map.of("language", "Java", "problem", "Implement sorting");
-
+ void render_withNonexistentTemplate_throwsException() {
+ Map<String, String> variables = Map.of("key", "value");
+
assertThatThrownBy(() -> templateService.render("/nonexistent/template.st", variables)).isInstanceOf(RuntimeException.class)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| void render_withStringVariables_replacesCorrectly() { | |
| String template = "Programming language: {{language}}, Problem: {{problem}}"; | |
| Map<String, String> variables = Map.of("language", "Java", "problem", "Implement sorting"); | |
| assertThatThrownBy(() -> templateService.render("/nonexistent/template.st", variables)).isInstanceOf(RuntimeException.class) | |
| .hasMessageContaining("Failed to load template"); | |
| } | |
| @Test | |
| void render_withNonexistentTemplate_throwsException() { | |
| Map<String, String> variables = Map.of("key", "value"); | |
| assertThatThrownBy(() -> templateService.render("/nonexistent/template.st", variables)).isInstanceOf(RuntimeException.class) | |
| .hasMessageContaining("Failed to load template"); | |
| } |
🤖 Prompt for AI Agents
In
src/test/java/de/tum/cit/aet/artemis/hyperion/service/HyperionPromptTemplateServiceTest.java
around lines 31 to 38, remove the unused local variables `template` and
`variables` and rename the test method to reflect that it verifies failure when
loading a non-existent template (for example rename to
`render_nonexistentTemplate_throwsRuntimeException`), leaving the existing
assertion that calling templateService.render("/nonexistent/template.st",
variables) throws a RuntimeException with a message containing "Failed to load
template" (adjust the variables argument to a minimal empty or null map if
needed).
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
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
Screenshots
Hyperion_Code_Generation_with_Artemis.mp4
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests