Integrated code lifecycle: Improve build status updates for users
#9818
Conversation
…-build-notification # Conflicts: # src/main/webapp/app/exercises/shared/result/updating-result.component.html # src/main/webapp/app/exercises/shared/result/updating-result.component.ts
github-actions
bot
added
tests
server
WalkthroughThe pull request introduces significant enhancements to various components related to build job processing and timing information. Key modifications include updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmdsrc/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.
🔧 eslint
src/main/webapp/app/exercises/programming/participate/programming-submission.service.tsOops! Something went wrong! :( ESLint: 9.14.0 TypeError: Error while loading rule '@typescript-eslint/no-unused-expressions': Cannot read properties of undefined (reading 'allowShortCircuit') Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🧹 Outside diff range and nitpick comments (45)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
170-175: Clarify the need for bothsetExerciseandsetProgrammingExercisemethodsIn lines 173~ and 174~, both
setExercise(exercise)andsetProgrammingExercise(exercise)are called onprogrammingExerciseParticipation. Please verify if both methods are necessary and that they update different properties. If they serve the same purpose, consider using only one to eliminate redundancy and improve code clarity.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (2)
400-403: Rename methodgetBuildAgentsCapacity()toupdateBuildAgentsCapacity()for clarityThe method
getBuildAgentsCapacity()updates thebuildAgentsCapacityandrunningBuildJobCountfields, rather than simply retrieving values. Renaming it toupdateBuildAgentsCapacity()would better reflect its purpose and improve code readability.Apply this diff to rename the method and its usages:
- private void getBuildAgentsCapacity() { + private void updateBuildAgentsCapacity() { buildAgentsCapacity = buildAgentInformation.values().stream() .mapToInt(BuildAgentInformation::maxNumberOfConcurrentBuildJobs).sum(); runningBuildJobCount = buildAgentInformation.values().stream() .mapToInt(BuildAgentInformation::numberOfCurrentBuildJobs).sum(); } - this.getBuildAgentsCapacity(); + this.updateBuildAgentsCapacity();
378-398: Consider makingBuildAgentListenera static nested classThe inner class
BuildAgentListenerdoes not access any instance members of the enclosing class. Making it a static nested class can improve clarity and reduce unnecessary references to the enclosing class.Apply this diff:
- private class BuildAgentListener + private static class BuildAgentListener implements EntryAddedListener<String, BuildAgentInformation>, EntryRemovedListener<String, BuildAgentInformation>, EntryUpdatedListener<String, BuildAgentInformation> {src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts (4)
Line range hint
713-745: Ensure correct handling of build timing informationWhen processing the pending submission, the code tries to access
submission.estimatedCompletionDateandsubmission.buildStartDate, which might be undefined. Ensure that these properties are available or handle the undefined case to prevent errors.Apply this diff to handle undefined properties:
let buildTimingInfo: BuildTimingInfo | undefined = { estimatedCompletionDate: submission.estimatedCompletionDate, buildStartDate: submission.buildStartDate, }; -buildTimingInfo = buildTimingInfo ?? this.startedProcessingCache.get(submission.commitHash!); +buildTimingInfo = buildTimingInfo || this.startedProcessingCache.get(submission.commitHash!); this.removeSubmissionFromProcessingCache(submission.commitHash!);Additionally, verify that
submission.commitHashis defined before using it.
896-903: Consider access modifiers for new methodsThe methods
setLocalCIProfileandgetIsLocalCIProfilecan be marked asprivateorprotectedif they are not intended to be accessed from outside the service.Apply this diff to adjust the access modifiers:
-public setLocalCIProfile(isLocalCIProfile: boolean) { +private setLocalCIProfile(isLocalCIProfile: boolean) { this.isLocalCIProfile = isLocalCIProfile; } -public getIsLocalCIProfile() { +private getIsLocalCIProfile() { return this.isLocalCIProfile; }
73-74: Follow consistent formatting for commentsEnsure that there is a space after the comment delimiters for readability and consistency.
Apply this diff to adjust the comment formatting:
-//Default value: 30 seconds. +// Default value: 30 seconds.
268-294: Handle potential race conditions in websocket message processingWhen processing websocket messages, there could be a race condition if messages arrive out of order. Consider adding logic to handle such scenarios to ensure that the state remains consistent.
src/main/webapp/app/entities/programming/submission-processing-dto.ts (1)
3-9: Consider adding fields for comprehensive build status tracking.Based on the PR objectives to improve build status updates, consider adding these fields:
status: BuildStatus- enum for states like 'QUEUED', 'PROCESSING', 'COMPLETED'queuePosition?: number- position in the build queueestimatedQueueDuration?: number- expected wait time in queueThis would provide more detailed status information to users.
src/main/webapp/app/exercises/shared/result/updating-result.component.html (1)
15-17: LGTM! Consider adding tooltips for timing information.The new timing properties (
estimatedCompletionDate,buildStartDate) and progress visualization (showProgressBar) effectively support the build queue duration estimation system. These additions will help users better understand build timelines.Consider enhancing the user experience further by adding tooltips that explain the estimated completion time in a user-friendly format (e.g., "Expected to complete in ~5 minutes").
[estimatedCompletionDate]="estimatedCompletionDate" [buildStartDate]="buildStartDate" - [showProgressBar]="showProgressBar" + [showProgressBar]="showProgressBar" + [matTooltip]="getEstimatedTimeTooltip()" + matTooltipPosition="above"src/main/java/de/tum/cit/aet/artemis/buildagent/dto/JobTimingInfo.java (2)
13-14: Document the unit of measurement for estimatedDurationThe
estimatedDurationfield is of typelongbut its unit of measurement (seconds, milliseconds, etc.) is not documented. This could lead to inconsistent usage across the codebase.Add a Javadoc comment to specify the unit:
@JsonIgnoreProperties(ignoreUnknown = true) @JsonInclude(JsonInclude.Include.NON_EMPTY) +/** + * Record representing timing information for build jobs. + * + * @param submissionDate When the job was submitted + * @param buildStartDate When the build started processing + * @param buildCompletionDate When the build completed + * @param estimatedCompletionDate Expected completion time + * @param estimatedDuration Expected duration in milliseconds + */ public record JobTimingInfo(ZonedDateTime submissionDate, ...
13-14: Breaking Change: Plan migration strategy for shared data structuresAs noted in the comment, this change affects shared data structures in Hazelcast/Redis. A migration strategy is needed to handle existing serialized data.
Consider the following recommendations:
- Document the migration steps in release notes
- Implement a data migration script
- Consider adding version information to the serialized data
- Plan for backward compatibility during the migration period
src/main/webapp/app/entities/programming/programming-submission.model.ts (1)
9-11: Consider enhancing type safety and documentation.While the implementation is correct, we could improve it further:
- Add JSDoc comments to document the purpose and usage of each new property
- Consider using a union type for more precise state management
Here's a suggested enhancement:
+ /** + * Indicates whether the submission is currently being processed by the build system. + * Used to differentiate between queued and active processing states. + */ public isProcessing?: boolean; + /** + * Timestamp when the build process started. + * Used for calculating actual build duration. + */ public buildStartDate?: dayjs.Dayjs; + /** + * Predicted completion time based on build queue analysis. + * Used to display estimated wait time to users. + */ public estimatedCompletionDate?: dayjs.Dayjs;Consider adding a type for better state management:
type SubmissionState = 'QUEUED' | 'PROCESSING' | 'COMPLETED' | 'FAILED';src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseBuildConfigRepository.java (1)
21-23: Add Javadoc to document the exception behavior.The implementation looks good and follows the repository's pattern for handling missing entities. Consider adding documentation to clarify the exception thrown when the config is not found.
Add this documentation above the method:
+ /** + * Retrieves a programming exercise build configuration by its programming exercise ID. + * + * @param programmingExerciseId the ID of the programming exercise + * @return the build configuration + * @throws EntityNotFoundException if no build configuration exists for the given programming exercise ID + */ default ProgrammingExerciseBuildConfig findByProgrammingExerciseIdElseThrow(Long programmingExerciseId) {src/main/java/de/tum/cit/aet/artemis/exercise/dto/SubmissionDTO.java (2)
18-19: LGTM! Consider adding field documentation.The new fields appropriately support the build status tracking requirements. Good choice using ZonedDateTime for timestamp fields and primitive boolean for isProcessing.
Consider adding Javadoc for the new fields to document their purpose:
/** * DTO containing {@link Submission} information. * This does not include large reference attributes in order to send minimal data to the client. * * @param isProcessing indicates if the submission is currently being processed * @param buildStartDate the date when the build started processing * @param estimatedCompletionDate the estimated date when the build will complete */
27-37: Consider grouping timing parameters into a separate record.To improve maintainability and reduce parameter count, consider extracting timing-related fields into a separate record.
Create a new record:
public record SubmissionTiming( boolean isProcessing, ZonedDateTime buildStartDate, ZonedDateTime estimatedCompletionDate ) {}Then refactor the method:
-public static SubmissionDTO of(Submission submission, boolean isProcessing, ZonedDateTime buildStartDate, ZonedDateTime estimatedCompletionDate) { +public static SubmissionDTO of(Submission submission, SubmissionTiming timing) { if (submission instanceof ProgrammingSubmission programmingSubmission) { return new SubmissionDTO(/*...*/, - isProcessing, buildStartDate, estimatedCompletionDate); + timing.isProcessing(), timing.buildStartDate(), timing.estimatedCompletionDate()); } return new SubmissionDTO(/*...*/, - false, null, null); + SubmissionTiming.DEFAULT); // Add a static DEFAULT instance with false/null valuessrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java (1)
39-39: Consider using builder pattern for better readabilityThe
SubmissionDTO.of()call with multiple boolean and null parameters reduces code clarity. Consider using the builder pattern or creating a more explicit factory method for finished build jobs.Example approach:
-SubmissionDTO submissionDTO = result.getSubmission() == null ? null : SubmissionDTO.of(result.getSubmission(), false, null, null); +SubmissionDTO submissionDTO = result.getSubmission() == null ? null : + SubmissionDTO.finishedBuildOf(result.getSubmission()); // New factory method specifically for finished buildssrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobQueueItem.java (2)
Line range hint
1-15: Consider implementing versioning strategy for shared data structure changes.Given that this is a shared data structure between core and build agent nodes, and changes require migration or clearing of Hazelcast/Redis data, consider implementing a versioning strategy to handle data structure evolution more gracefully.
Consider:
- Adding version field to the record
- Implementing migration scripts
- Adding documentation about migration steps in release notes
32-34: Consider adding validation for timing information consistency.The constructor should ensure that the timing information is consistent (e.g., completion dates are after start dates).
Consider adding validation:
public BuildJobQueueItem(BuildJobQueueItem queueItem, ZonedDateTime buildCompletionDate, BuildStatus status) { + Objects.requireNonNull(buildCompletionDate, "buildCompletionDate must not be null"); + Objects.requireNonNull(status, "status must not be null"); + if (queueItem.jobTimingInfo.buildStartDate() != null && buildCompletionDate.isBefore(queueItem.jobTimingInfo.buildStartDate())) { + throw new IllegalArgumentException("buildCompletionDate must be after buildStartDate"); + } this(queueItem.id(), queueItem.name(), queueItem.buildAgent(), queueItem.participationId(), queueItem.courseId(), queueItem.exerciseId(), queueItem.retryCount(), queueItem.priority(), status, queueItem.repositoryInfo(), new JobTimingInfo(queueItem.jobTimingInfo.submissionDate(), queueItem.jobTimingInfo.buildStartDate(), buildCompletionDate, queueItem.jobTimingInfo.estimatedCompletionDate(), queueItem.jobTimingInfo.estimatedDuration()), queueItem.buildConfig(), null); }src/main/java/de/tum/cit/aet/artemis/programming/dto/ResultDTO.java (1)
Line range hint
27-29: Consider implementing server-side result string calculationThe TODO comment raises a valid architectural point about moving result string calculations from client to server side. This would improve consistency across different clients and simplify client-side logic.
Would you like me to help create a GitHub issue to track the implementation of server-side result string calculation? I can provide a detailed implementation plan that includes:
- Adding result string calculation logic to the server
- Updating the DTO to include the calculated string
- Removing client-side calculation logic
src/main/webapp/app/exercises/programming/participate/code-editor-student-container.component.html (1)
78-81: Progress bar integration aligns with PR objectivesThe addition of
[showProgressBar]="true"to the<jhi-updating-result>component enhances the user experience by providing visual feedback during build status updates, which aligns well with the PR's goal of improving build status visibility.Consider making the progress bar visibility configurable based on the submission state (queued vs. processing) to provide more granular feedback to users.
- [showProgressBar]="true" + [showProgressBar]="participation.submissionState === 'QUEUED' || participation.submissionState === 'PROCESSING'"src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (2)
42-43: Update constructor's javadoc to include the new parameterThe constructor injection is correctly implemented, but the javadoc needs to be updated to include the
programmingMessagingServiceparameter./** * Instantiates a new Local ci queue websocket service. * * @param hazelcastInstance the hazelcast instance * @param localCIWebsocketMessagingService the local ci build queue websocket service * @param sharedQueueManagementService the local ci shared build job queue service + * @param programmingMessagingService the programming messaging service */Also applies to: 56-56, 60-60
154-157: Add debug logging for build processing notificationsConsider adding debug logging to track notification delivery, similar to other debug logs in the service.
private void notifyUserAboutBuildProcessing(long exerciseId, long participationId, String commitHash, ZonedDateTime buildStartDate, ZonedDateTime estimatedCompletionDate) { + log.debug("Notifying user about build processing - exercise: {}, participation: {}, estimated completion: {}", exerciseId, participationId, estimatedCompletionDate); var submissionProcessingDTO = new SubmissionProcessingDTO(exerciseId, participationId, commitHash, buildStartDate, estimatedCompletionDate); programmingMessagingService.notifyUserAboutSubmissionProcessing(submissionProcessingDTO, exerciseId, participationId); }src/main/webapp/app/exercises/shared/result/result.component.html (2)
7-15: LGTM! Consider adding ARIA attributes for better accessibility.The implementation correctly differentiates the queued state and follows the new Angular syntax guidelines. The conditional display of estimated time enhances user experience.
Consider adding
aria-labelto improve accessibility:- <span class="text-primary"> + <span class="text-primary" role="status" aria-label="{{ 'artemisApp.editor.queued' | translate }}">
16-37: Enhance progress bar accessibility.The progress bar implementation looks good, but could benefit from better accessibility support.
Consider these accessibility enhancements:
- <div class="progress position-relative" role="progressbar" [style.min-width.px]="200" [style.height.px]="30"> + <div + class="progress position-relative" + role="progressbar" + [attr.aria-valuenow]="progressBarValue" + aria-valuemin="0" + aria-valuemax="100" + [attr.aria-label]="'artemisApp.editor.eta' | translate" + [style.min-width.px]="200" + [style.height.px]="30">src/main/webapp/i18n/en/editor.json (1)
34-36: LGTM with a minor suggestion for the ETA info text.The new translation keys are well-integrated and align perfectly with the PR objectives to improve build status visibility. The messages are clear and consistent with the existing style.
Consider making the ETA explanation slightly more specific:
- "etaInfo": "Estimated time until build is finished. This is an estimate and can vary.", + "etaInfo": "Estimated time until your build is finished. This time is calculated based on current queue status and may vary.",src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingSubmissionRepository.java (2)
162-170: Consider consolidating duplicate query methods.This query is very similar to the existing
findByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudentsmethod, but fetches fewer associations. Consider:
- Renaming this method to indicate it's a "light" version
- Or creating a single method with optional fetch joins based on a parameter
Example refactor to combine both methods:
-@Query(""" - SELECT s - FROM ProgrammingSubmission s - LEFT JOIN FETCH s.participation p - WHERE p.id = :participationId - AND s.commitHash = :commitHash - ORDER BY s.id DESC - """) -List<ProgrammingSubmission> findByParticipationIdAndCommitHashOrderByIdDesc( - @Param("participationId") long participationId, - @Param("commitHash") String commitHash); +@Query(""" + SELECT s + FROM ProgrammingSubmission s + LEFT JOIN FETCH s.participation p + LEFT JOIN FETCH CASE WHEN :includeFeedbacks = true THEN s.results END r + LEFT JOIN FETCH CASE WHEN :includeFeedbacks = true THEN r.feedbacks END f + LEFT JOIN FETCH CASE WHEN :includeFeedbacks = true THEN p.team END t + LEFT JOIN FETCH CASE WHEN :includeFeedbacks = true THEN t.students END + WHERE p.id = :participationId + AND s.commitHash = :commitHash + ORDER BY s.id DESC + """) +List<ProgrammingSubmission> findByParticipationIdAndCommitHash( + @Param("participationId") long participationId, + @Param("commitHash") String commitHash, + @Param("includeFeedbacks") boolean includeFeedbacks);
172-174: Consider using Optional for null safety.The method returns null when no submission is found. Consider using Optional to make this explicit in the method signature, consistent with other similar methods in this repository (e.g.,
findByResultId).-default ProgrammingSubmission findFirstByParticipationIdAndCommitHashOrderByIdDesc( - long participationId, String commitHash) { - return findByParticipationIdAndCommitHashOrderByIdDesc(participationId, commitHash) - .stream().findFirst().orElse(null); -} +default Optional<ProgrammingSubmission> findFirstByParticipationIdAndCommitHashOrderByIdDesc( + long participationId, String commitHash) { + return findByParticipationIdAndCommitHashOrderByIdDesc(participationId, commitHash) + .stream().findFirst(); +}src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java (2)
201-203: Consider using a DTO for the responseInstead of returning a raw ZonedDateTime, consider creating a dedicated DTO to encapsulate the queue duration estimation. This would provide more flexibility for adding additional metadata in the future without breaking API compatibility.
Example DTO:
public record QueueDurationEstimationDTO( ZonedDateTime estimatedStartTime, long estimatedDurationInSeconds, String status ) {}
195-200: Enhance JavaDoc with more detailsThe current JavaDoc could be more descriptive about:
- What the estimated queue duration represents (start time, completion time?)
- The return value format and timezone
- Possible error responses
Example enhancement:
/** * Returns the estimated queue duration for a build job. * * @param participationId the id of the participation * @return ResponseEntity containing the estimated start time in UTC * @throws BadRequestException if participationId is invalid */src/main/webapp/app/exercises/shared/result/updating-result.component.ts (1)
176-185: Consider decoupling CI profile check from timing updatesThe timing info update being tied to
getIsLocalCIProfile()might limit functionality if timing information becomes available in other CI environments.Consider extracting the CI profile check to a more flexible configuration:
- if (this.submissionService.getIsLocalCIProfile()) { + if (this.submissionService.getIsLocalCIProfile() || this.submissionService.supportsBuildTiming()) { this.updateBuildTimingInfo(submissionState, buildTimingInfo); }src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java (2)
88-93: Add documentation and validation for new fields.The new fields lack documentation explaining their purpose and constraints. Consider:
- Adding Javadoc to explain the purpose and unit of measurement for each field
- Adding validation constraints (e.g.,
@Min(0)) to ensure non-negative values+ /** + * The total duration of successful builds in seconds. + * This metric helps in estimating future build durations. + */ + @Min(0) @Column(name = "build_duration_seconds") private long buildDurationSeconds = 0; + /** + * The total number of successful builds. + * Used in conjunction with buildDurationSeconds to calculate average build duration. + */ + @Min(0) @Column(name = "successful_build_count") private long successfulBuildCount = 0;
303-318: Update toString() method to include new fields.The new fields should be included in the toString() method to maintain consistency and aid in debugging.
@Override public String toString() { return "BuildJobConfig{" + "id=" + getId() + ", sequentialTestRuns=" + sequentialTestRuns + ", branch='" + branch + '\'' + ", buildPlanConfiguration='" + buildPlanConfiguration + '\'' + ", buildScript='" + buildScript + '\'' + ", checkoutSolutionRepository=" + checkoutSolutionRepository + ", checkoutPath='" + testCheckoutPath + '\'' + ", timeoutSeconds=" + timeoutSeconds + ", dockerFlags='" + dockerFlags + '\'' + ", testwiseCoverageEnabled=" + testwiseCoverageEnabled - + ", theiaImage='" + theiaImage + '\'' + ", allowBranching=" + allowBranching + ", branchRegex='" + branchRegex + '\'' + '}'; + + ", theiaImage='" + theiaImage + '\'' + ", allowBranching=" + allowBranching + ", branchRegex='" + branchRegex + '\'' + + ", buildDurationSeconds=" + buildDurationSeconds + ", successfulBuildCount=" + successfulBuildCount + '}'; }src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIServiceTest.java (1)
Line range hint
73-92: Enhance build status test assertions and DB performance tracking.The test method
testReturnCorrectBuildStatuscould benefit from:
- More specific assertions about the timing information
- DB query count tracking as per coding guidelines
Consider enhancing the test:
+ @Test + @WithMockUser(username = "instructor1", roles = "INSTRUCTOR") + @MonitorDBQueries // Add DB query monitoring void testReturnCorrectBuildStatus() { // ... existing setup ... // Add specific timing assertions + assertThat(job1.getJobTimingInfo()) + .satisfies(timing -> { + assertThat(timing.estimatedCompletionDate()).isNotNull(); + assertThat(timing.estimatedDuration()).isPositive(); + }); // ... existing assertions ... }src/test/javascript/spec/component/exercises/shared/result.spec.ts (1)
226-233: Good test case, but consider enhancing it.The test correctly verifies interval creation, but could be improved for better coverage and cleanup.
Consider these enhancements:
- Add interval cleanup in afterEach to prevent memory leaks:
afterEach(() => { jest.restoreAllMocks(); + if (component.estimatedDurationInterval) { + clearInterval(component.estimatedDurationInterval); + } });
- Add more assertions to verify interval behavior:
it('should trigger Interval creation on estimatedCompletionDate change', () => { component.estimatedCompletionDate = dayjs().add(20, 'seconds'); component.ngOnChanges({ estimatedCompletionDate: { previousValue: undefined, currentValue: component.estimatedCompletionDate, firstChange: true, isFirstChange: () => true }, }); expect(component.estimatedDurationInterval).toBeDefined(); + // Verify interval is cleared when estimatedCompletionDate is removed + component.ngOnChanges({ + estimatedCompletionDate: { previousValue: component.estimatedCompletionDate, currentValue: undefined, firstChange: false, isFirstChange: () => false }, + }); + expect(component.estimatedDurationInterval).toBeUndefined(); });src/test/javascript/spec/component/shared/updating-result.component.spec.ts (2)
51-54: Consider adding edge cases to test dataWhile the current test data is good for happy path testing, consider adding edge cases such as:
- Past estimated completion dates
- Null or undefined dates
- Very short/long duration estimates
const buildTimingInfo: BuildTimingInfo = { buildStartDate: dayjs().subtract(10, 'second'), estimatedCompletionDate: dayjs().add(10, 'second'), + // Add more test cases: + // pastEstimation: { + buildStartDate: dayjs().subtract(20, 'second'), + estimatedCompletionDate: dayjs().subtract(10, 'second'), + }, + // nullDates: { + buildStartDate: null, + estimatedCompletionDate: null, + }, };
Line range hint
1-267: Overall test coverage is good but could be enhancedThe test suite effectively covers the new build queue functionality and timing updates. However, consider:
- Adding more edge cases for timing data
- Improving error scenario coverage
- Adding explicit cleanup verification
- Testing network error handling
These improvements would make the test suite more robust and comprehensive.
src/main/webapp/app/exercises/shared/result/result.component.ts (4)
58-58: LGTM! Consider adding JSDoc comments.The new input properties are well-typed and follow Angular naming conventions. Consider adding JSDoc comments to document their purpose and expected values.
Example documentation:
/** Indicates if the build job is currently in queue */ @Input() isQueued = false; /** Expected completion time of the build job */ @Input() estimatedCompletionDate?: dayjs.Dayjs; /** Time when the build job started processing */ @Input() buildStartDate?: dayjs.Dayjs; /** Controls the visibility of the progress bar */ @Input() showProgressBar = false;Also applies to: 68-70
80-82: Initialize properties and add type annotations.The new properties should be initialized to prevent undefined behavior and have explicit type annotations for better maintainability.
-estimatedRemaining: number; -progressBarValue: number; -estimatedDurationInterval: ReturnType<typeof setInterval>; +/** Remaining time in seconds until estimated completion */ +estimatedRemaining = 0; +/** Progress bar completion percentage (0-100) */ +progressBarValue = 0; +/** Interval handle for updating estimated duration */ +estimatedDurationInterval?: ReturnType<typeof setInterval>;
Line range hint
188-213: Fix potential issues in progress calculation logic.Several issues need to be addressed:
- Clear existing interval before setting up a new one to prevent memory leaks
- Add null check for buildStartDate
- Add protection against division by zero
- Extract magic number 100 to a constant
if (changes.estimatedCompletionDate && this.estimatedCompletionDate) { - clearInterval(this.estimatedDurationInterval); + if (this.estimatedDurationInterval) { + clearInterval(this.estimatedDurationInterval); + } + + if (!this.buildStartDate) { + return; + } + + const PROGRESS_MAX = 100; this.estimatedDurationInterval = setInterval(() => { this.estimatedRemaining = Math.max(0, dayjs(this.estimatedCompletionDate).diff(dayjs(), 'seconds')); const estimatedDuration = dayjs(this.estimatedCompletionDate).diff(dayjs(this.buildStartDate), 'seconds'); - this.progressBarValue = Math.round((1 - this.estimatedRemaining / estimatedDuration) * 100); + this.progressBarValue = estimatedDuration > 0 + ? Math.min(Math.round((1 - this.estimatedRemaining / estimatedDuration) * PROGRESS_MAX), PROGRESS_MAX) + : 0; if (this.estimatedRemaining <= 0) { clearInterval(this.estimatedDurationInterval); } }, 1000); }
Line range hint
1-349: Add unit tests for new functionality.Consider adding unit tests to verify:
- Progress calculation logic
- State transitions between queued and building states
- Cleanup of intervals
- Edge cases in estimated time calculations
Would you like me to help generate the unit test cases?
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java (1)
197-197: Consider using builder pattern for JobTimingInfo creation.The current constructor call with multiple null parameters reduces code readability and makes it harder to understand the purpose of each parameter. Consider implementing a builder pattern for
JobTimingInfoto make the code more maintainable and self-documenting.Consider creating a builder for JobTimingInfo:
public record JobTimingInfo( ZonedDateTime submissionDate, ZonedDateTime processingStartDate, ZonedDateTime processingEndDate, ZonedDateTime estimatedCompletionDate, long estimatedDuration ) { public static class Builder { private ZonedDateTime submissionDate; private ZonedDateTime processingStartDate; private ZonedDateTime processingEndDate; private ZonedDateTime estimatedCompletionDate; private long estimatedDuration; public Builder submissionDate(ZonedDateTime date) { this.submissionDate = date; return this; } public Builder estimatedDuration(long duration) { this.estimatedDuration = duration; return this; } public JobTimingInfo build() { return new JobTimingInfo( submissionDate, processingStartDate, processingEndDate, estimatedCompletionDate, estimatedDuration ); } } }Then update the instantiation:
- JobTimingInfo jobTimingInfo = new JobTimingInfo(submissionDate, null, null, null, estimatedDuration); + JobTimingInfo jobTimingInfo = new JobTimingInfo.Builder() + .submissionDate(submissionDate) + .estimatedDuration(estimatedDuration) + .build();src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
98-99: Consider using Builder pattern for test data setup.The current test data setup is verbose and could be hard to maintain. Consider implementing a Builder pattern for BuildJobQueueItem and BuildAgentInformation to make the test setup more readable and maintainable.
Example implementation:
public class BuildJobQueueItemBuilder { private String id = "default-id"; private String name = "default-name"; private BuildAgentDTO buildAgent; // ... other fields public static BuildJobQueueItemBuilder aJob() { return new BuildJobQueueItemBuilder(); } public BuildJobQueueItemBuilder withId(String id) { this.id = id; return this; } public BuildJobQueueItem build() { return new BuildJobQueueItem(id, name, buildAgent, ...); } } // Usage in test: job1 = BuildJobQueueItemBuilder.aJob() .withId("1") .withName("job1") .withBuildAgent(buildAgent) .build();src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
Line range hint
1-1: Consider adding timing metrics collection.To further improve build status updates, consider:
- Collecting historical timing data for better estimations
- Adding metrics for queue wait times and processing times
- Implementing a feedback mechanism to adjust estimations based on actual completion times
Example metrics implementation:
@Service public class BuildMetricsService { private final MeterRegistry registry; public void recordQueueTime(Duration duration) { registry.timer("build.queue.time").record(duration); } public void recordProcessingTime(Duration duration) { registry.timer("build.processing.time").record(duration); } public void recordEstimationAccuracy(Duration estimated, Duration actual) { registry.gauge("build.estimation.accuracy", actual.getSeconds() - estimated.getSeconds()); } }src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (1)
517-544: Enhance test readability and robustness.While the test logic is sound, consider these improvements:
- Extract magic number 24 into a constant with a descriptive name explaining the calculation (20 seconds + 20% buffer).
- Add assertions to verify that buildStartDate is not null before using it in timing calculations.
- Consider adding comments explaining the timing calculations for better maintainability.
Here's a suggested refactor:
+ private static final int BUILD_DURATION_SECONDS = 20; + private static final double DURATION_BUFFER_FACTOR = 1.2; + private static final int EXPECTED_DURATION = (int) (BUILD_DURATION_SECONDS * DURATION_BUFFER_FACTOR); + @Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") void testBuildJobTimingInfo() { // Pause build agent processing sharedQueueProcessingService.removeListenerAndCancelScheduledFuture(); ProgrammingExerciseBuildConfig buildConfig = programmingExercise.getBuildConfig(); - buildConfig.setBuildDurationSeconds(20); + buildConfig.setBuildDurationSeconds(BUILD_DURATION_SECONDS); programmingExerciseBuildConfigRepository.save(buildConfig); ProgrammingExerciseStudentParticipation studentParticipation = localVCLocalCITestService.createParticipation(programmingExercise, student1Login); localVCServletService.processNewPush(commitHash, studentAssignmentRepository.originGit.getRepository()); await().until(() -> queuedJobs.stream().anyMatch(buildJobQueueItem -> buildJobQueueItem.buildConfig().commitHashToBuild().equals(commitHash) && buildJobQueueItem.participationId() == studentParticipation.getId())); BuildJobQueueItem item = queuedJobs.stream().filter(i -> i.buildConfig().commitHashToBuild().equals(commitHash) && i.participationId() == studentParticipation.getId()) .findFirst().orElseThrow(); - assertThat(item.jobTimingInfo().estimatedDuration()).isEqualTo(24); + // Verify estimated duration includes buffer time + assertThat(item.jobTimingInfo().estimatedDuration()).isEqualTo(EXPECTED_DURATION); sharedQueueProcessingService.init(); await().until(() -> processingJobs.values().stream().anyMatch(buildJobQueueItem -> buildJobQueueItem.buildConfig().commitHashToBuild().equals(commitHash) && buildJobQueueItem.participationId() == studentParticipation.getId())); item = processingJobs.values().stream().filter(i -> i.buildConfig().commitHashToBuild().equals(commitHash) && i.participationId() == studentParticipation.getId()) .findFirst().orElseThrow(); - assertThat(item.jobTimingInfo().estimatedDuration()).isEqualTo(24); + assertThat(item.jobTimingInfo().estimatedDuration()).isEqualTo(EXPECTED_DURATION); + // Verify buildStartDate is set + assertThat(item.jobTimingInfo().buildStartDate()).isNotNull(); + // Verify estimated completion date is correctly calculated assertThat(item.jobTimingInfo().estimatedCompletionDate()).isCloseTo(item.jobTimingInfo().buildStartDate().plusSeconds(24), within(500, ChronoUnit.MILLIS)); }src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
241-242: Consider documenting the side effect.The code sets the participation to null before creating the DTO. While this is intentional to reduce response size, it's a side effect that should be documented.
Add a more descriptive comment:
- // Remove participation, is not needed in the response. + // Set participation to null to reduce response payload size since it's not needed in the client
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241101120000_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (37)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobQueueItem.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/JobTimingInfo.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/dto/SubmissionDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/dto/ResultDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/dto/SubmissionProcessingDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseBuildConfigRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingSubmissionRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java(2 hunks)src/main/webapp/app/entities/programming/programming-submission.model.ts(1 hunks)src/main/webapp/app/entities/programming/submission-processing-dto.ts(1 hunks)src/main/webapp/app/exercises/programming/participate/code-editor-student-container.component.html(1 hunks)src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts(16 hunks)src/main/webapp/app/exercises/shared/result/result.component.html(1 hunks)src/main/webapp/app/exercises/shared/result/result.component.ts(6 hunks)src/main/webapp/app/exercises/shared/result/result.utils.ts(3 hunks)src/main/webapp/app/exercises/shared/result/updating-result.component.html(1 hunks)src/main/webapp/app/exercises/shared/result/updating-result.component.ts(5 hunks)src/main/webapp/app/overview/submission-result-status.module.ts(1 hunks)src/main/webapp/i18n/de/editor.json(1 hunks)src/main/webapp/i18n/en/editor.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java(6 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(5 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIServiceTest.java(1 hunks)src/test/javascript/spec/component/exercises/shared/result.spec.ts(1 hunks)src/test/javascript/spec/component/shared/updating-result.component.spec.ts(6 hunks)src/test/javascript/spec/helpers/mocks/service/mock-programming-submission.service.ts(2 hunks)src/test/javascript/spec/service/programming-submission.service.spec.ts(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/programming/dto/SubmissionProcessingDTO.java
🧰 Additional context used
📓 Path-based instructions (35)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobQueueItem.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/JobTimingInfo.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/exercise/dto/SubmissionDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/dto/ResultDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseBuildConfigRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingSubmissionRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/entities/programming/programming-submission.model.ts (1)
src/main/webapp/app/entities/programming/submission-processing-dto.ts (1)
src/main/webapp/app/exercises/programming/participate/code-editor-student-container.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts (1)
src/main/webapp/app/exercises/shared/result/result.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exercises/shared/result/result.component.ts (1)
src/main/webapp/app/exercises/shared/result/result.utils.ts (1)
src/main/webapp/app/exercises/shared/result/updating-result.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exercises/shared/result/updating-result.component.ts (1)
src/main/webapp/app/overview/submission-result-status.module.ts (1)
src/main/webapp/i18n/de/editor.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/javascript/spec/component/exercises/shared/result.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/shared/updating-result.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/helpers/mocks/service/mock-programming-submission.service.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/service/programming-submission.service.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🪛 Biome
src/test/javascript/spec/service/programming-submission.service.spec.ts
[error] 151-151: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 170-170: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (44)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (3)
55-65: Proper use of constructor injection for ParticipationRepository.
Injecting ParticipationRepository via the constructor aligns with dependency injection best practices and improves testability.
170-172: getSubmissionProcessingTopicForTAAndAbove is correctly implemented.
The method correctly constructs the topic string for TA and above, ensuring notifications are sent to the appropriate recipients.
87-88: Ensure consistent usage of notifyUserAboutSubmission methods.
The addition of the isProcessing parameter and overloading notifyUserAboutSubmission ensures clarity in notifications. Verify that all calls to notifyUserAboutSubmission pass the correct isProcessing value to maintain consistent behavior.
To confirm consistent usage, run the following script to find all invocations of notifyUserAboutSubmission:
✅ Verification successful
Let me analyze the results from the rg command to verify the consistent usage of notifyUserAboutSubmission.
The search results show:
- Two method definitions in
ProgrammingMessagingService.java:- One with two parameters that delegates to the three-parameter version with
false - One with three parameters (including
isProcessing)
- One with two parameters that delegates to the three-parameter version with
- All other invocations across the codebase use the two-parameter version, which correctly defaults to
isProcessing=false
The usage is consistent across the codebase.
All calls to notifyUserAboutSubmission either:
- Use the two-parameter version, which internally sets
isProcessing=false - Or would explicitly pass the
isProcessingparameter using the three-parameter version if needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to notifyUserAboutSubmission methods
# Search for method calls to notifyUserAboutSubmission with or without the new parameter
rg --no-heading --line-number 'notifyUserAboutSubmission\(' -A 1
Length of output: 3635
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (2)
269-269: Good practice: Changed saveFinishedBuildJob visibility to private
Making the saveFinishedBuildJob method private restricts its access to within the class, adhering to the principle of least privilege. This enhances encapsulation and reduces the potential for misuse outside this class.
97-97: Appropriate use of constructor injection for dependencies
Assigning programmingExerciseBuildConfigRepository via constructor injection in line 97~ follows dependency injection best practices, improving the testability and maintainability of the service.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (1)
346-352:
Replace getLast() with get(participationBuildJobIds.size() - 1) to avoid compilation error
In the method getIdOfQueuedJobFromParticipation, using getLast() on a List may lead to a compilation error since List does not have a getLast() method in Java. You should use get(participationBuildJobIds.size() - 1) instead to retrieve the last element.
Apply this diff to fix the issue:
- return participationBuildJobIds.getLast();
+ return participationBuildJobIds.get(participationBuildJobIds.size() - 1);Likely invalid or redundant comment.
src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts (4)
856-857: Ensure all subscriptions are properly unsubscribed in unsubscribeAllWebsocketTopics
In the unsubscribeAllWebsocketTopics method, make sure that all active subscriptions, especially those related to the queue estimate timers, are properly unsubscribed to prevent memory leaks.
16-19: Ensure correct module imports and paths
The added imports should be reviewed to ensure that the modules exist and the paths are correct. Also, verify that these imports are necessary and used within the service.
184-187: Verify usage of fetchQueueReleaseDateEstimationByParticipationId
The method fetchQueueReleaseDateEstimationByParticipationId is added but not used within the provided code. Ensure that it is utilized where necessary or remove it if it's not needed.
Run the following script to find usages of the method:
#!/bin/bash
# Description: Search for usages of `fetchQueueReleaseDateEstimationByParticipationId` in the codebase.
# Find all occurrences of the method name
rg 'fetchQueueReleaseDateEstimationByParticipationId\('448-451: Initialize buildTimingInfo when emitting queued submission
In the emitQueuedSubmission method, the buildTimingInfo property is not set, which might lead to issues when subscribers expect this information. Consider initializing buildTimingInfo appropriately.
Run the following script to check for usages of emitQueuedSubmission and ensure buildTimingInfo is handled correctly:
src/main/webapp/app/entities/programming/submission-processing-dto.ts (1)
1-1: LGTM! Proper ESM import for dayjs.
The ESM import is correctly used, which enables better tree-shaking for optimal bundle size.
src/main/webapp/app/exercises/shared/result/updating-result.component.html (1)
7-7: LGTM! Queue status property enhances user awareness.
The addition of the [isQueued] property aligns perfectly with the PR objective to differentiate between queued and processing states, improving transparency for users.
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/JobTimingInfo.java (1)
13-14: Verify usage of new timing fields across the codebase
Let's verify that all constructors and factory methods are updated to handle the new fields correctly.
✅ Verification successful
Based on the search results, I can now provide a final response as all constructors and factory methods have been found and analyzed:
All JobTimingInfo instantiations are properly handling the fields
The verification shows that all usages of JobTimingInfo correctly handle all five fields (submissionDate, buildStartDate, buildCompletionDate, estimatedCompletionDate, estimatedDuration) across the codebase:
- LocalCITriggerService initializes with null values for dates except submissionDate
- SharedQueueProcessingService properly copies all fields and updates buildCompletionDate
- BuildJobQueueItem correctly handles all fields in its factory methods
- Test classes demonstrate proper initialization with various combinations of dates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for JobTimingInfo instantiations
echo "Searching for JobTimingInfo constructors usage..."
rg "new JobTimingInfo\(" -A 2
# Search for methods that might need updates for the new fields
echo "Searching for methods handling JobTimingInfo..."
ast-grep --pattern 'class $_ {
$$$
$retType $method($_) {
$$$
JobTimingInfo
$$$
}
}'
Length of output: 6712
src/main/webapp/app/overview/submission-result-status.module.ts (2)
7-7: LGTM! Import statement follows Angular style guide.
The import statement follows proper naming conventions and module organization patterns.
10-10: Verify shared components for build status updates.
The addition of ArtemisSharedComponentModule aligns with the PR objectives for improving build status updates. Let's verify the required components are available.
✅ Verification successful
Build status components are correctly integrated through module dependencies
The verification confirms that build status functionality is properly integrated:
ArtemisProgrammingExerciseActionsModuleprovides the build status components (trigger build buttons and submission state)ArtemisSharedComponentModuleprovides necessary UI components for actions and buttons- Both modules are correctly imported in the
SubmissionResultStatusModule
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the shared components module contains necessary UI components for build status updates
# Check the shared component module for relevant components
ast-grep --pattern 'export class $status extends Component {
$$$
}' src/main/webapp/app/shared/components/**/*.ts
# Look for build status related templates
rg -g '*.html' -A 5 'build.*status|queue.*status' src/main/webapp/app/shared/components/
Length of output: 983
Script:
#!/bin/bash
# Let's try a different approach to verify build status components
# Check for build status related components in shared module
rg -l "BuildStatus|QueueStatus" src/main/webapp/app/shared/
# Check the shared module definition
cat src/main/webapp/app/shared/shared.module.ts
# Look for build status components in the overview module
rg -l "BuildStatus|QueueStatus" src/main/webapp/app/overview/
# Check the submission result status module
cat src/main/webapp/app/overview/submission-result-status.module.ts
Length of output: 5926
Script:
#!/bin/bash
# Let's check the shared component module and related components
# Check the shared component module definition
cat src/main/webapp/app/shared/components/shared-component.module.ts
# Look for build status related components in programming exercise actions
rg -l "BuildStatus|QueueStatus" src/main/webapp/app/exercises/programming/shared/actions/
# Check the programming exercise actions module
cat src/main/webapp/app/exercises/programming/shared/actions/programming-exercise-actions.module.ts
Length of output: 7193
src/main/webapp/app/entities/programming/programming-submission.model.ts (2)
9-11: LGTM! The new properties align well with the PR objectives.
The added properties effectively support the differentiation between queued and processing states while enabling build duration estimation. The implementation follows Angular style guidelines correctly.
Line range hint 22-23: Consider removing the TODO comment.
The TODO comment suggests this method could be removed after implementing websocket-based submission fetching. Since this PR implements websocket communication for build status updates, we should verify if this method is still needed.
Would you like me to help implement the websocket-based solution to remove this method?
src/test/javascript/spec/helpers/mocks/service/mock-programming-submission.service.ts (1)
3-3: LGTM: Import statement is correctly placed and necessary.
The dayjs import is required for the new method's return type signature.
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java (1)
39-39: Verify the purpose of the third null parameter
The third parameter passed to SubmissionDTO.of() is not clear from the context. Let's verify its purpose and usage.
✅ Verification successful
The third parameter is buildStartDate and its usage is verified
From the codebase analysis, the third and fourth parameters of SubmissionDTO.of() are buildStartDate and estimatedCompletionDate respectively. In FinishedBuildJobDTO, passing null for both parameters is correct because:
- The build is already finished, so we don't need a build start date
- An estimated completion date is not applicable for completed builds
This is consistent with other usages in the codebase, where these parameters are only populated for active builds, such as in ProgrammingExerciseParticipationResource and LocalCIQueueWebsocketService.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SubmissionDTO.of() method definition to understand parameters
ast-grep --pattern 'record SubmissionDTO {
$$$
static $$ of($$$) {
$$$
}
$$$
}'
# Search for other usages of SubmissionDTO.of() for comparison
rg "SubmissionDTO\.of\(" -A 2
Length of output: 3468
Script:
#!/bin/bash
# Search for SubmissionDTO record definition to understand its parameters
ast-grep --pattern 'record SubmissionDTO($$$)'
# Search for any documentation or comments above SubmissionDTO.of method
rg -B 5 "static \w+ of\(" src/main/java/de/tum/cit/aet/artemis/programming/dto/SubmissionDTO.java || true
Length of output: 391
Script:
#!/bin/bash
# Find the SubmissionDTO file first
fd "SubmissionDTO.java"
# Then try to find any references to buildStartDate or estimatedCompletionDate in DTOs
rg "buildStartDate|estimatedCompletionDate" --type java
Length of output: 10197
src/main/java/de/tum/cit/aet/artemis/programming/dto/ResultDTO.java (2)
Line range hint 1-73: Implementation follows best practices and guidelines
The implementation demonstrates excellent adherence to coding guidelines:
- Uses Java Records for immutable DTOs
- Follows single responsibility principle
- Implements minimal DTO pattern
- Maintains clear separation between domain and DTO
- Uses constructor injection
66-66: Verify the impact of null parameters in SubmissionDTO.of()
The updated method call includes false and two null parameters. While this aligns with the PR's objective to improve build status updates, we should verify that these null values don't cause issues.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Null parameters in SubmissionDTO.of() are used consistently across the codebase
The codebase shows consistent usage of null parameters in SubmissionDTO.of() calls across different services:
ProgrammingMessagingService:SubmissionDTO.of(submission, isProcessing, null, null)ResultDTO:SubmissionDTO.of(result.getSubmission(), false, null, null)FinishedBuildJobDTO:SubmissionDTO.of(result.getSubmission(), false, null, null)
The pattern of passing null for the last two parameters is standard practice in the codebase, and the implementation in SubmissionDTO is designed to handle these null values appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of SubmissionDTO.of to understand the parameter handling
ast-grep --pattern 'SubmissionDTO.of($_)'
# Search for null checks in SubmissionDTO
rg -A 5 "SubmissionDTO" | rg -A 5 "null"
Length of output: 7513
src/main/webapp/app/exercises/programming/participate/code-editor-student-container.component.html (2)
Line range hint 2-2: LGTM: Consistent usage of new Angular syntax
The template correctly uses the new @if directive instead of *ngIf throughout the file, following the coding guidelines.
Also applies to: 5-5, 11-11, 23-23, 28-28, 38-38, 76-76, 96-96
Line range hint 77-82: Verify integration with new timing properties
The <jhi-updating-result> component should be receiving and utilizing the new timing properties mentioned in the AI summary (isQueued, estimatedCompletionDate, buildStartDate) to properly display the progress bar and queue status.
✅ Verification successful
Let me generate another script to check the component implementation since we found the template usage but need to verify the property definitions.
Let me try one more search to specifically find the UpdatingResultComponent implementation:
Based on the code review and search results, I can now provide a final response:
The timing properties are correctly integrated in the UpdatingResultComponent
The component properly handles and utilizes the new timing properties:
isQueuedis set based on submission stateestimatedCompletionDateandbuildStartDateare updated inupdateBuildTimingInfo()method- The template correctly binds these properties to the result component
- The progress bar display is controlled via the
showProgressBarinput property
The implementation matches the expected behavior for displaying queue status and build progress.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the UpdatingResultComponent implements the new timing properties
# Check for the property definitions in the component
ast-grep --pattern 'class UpdatingResultComponent {
$$$
@Input() $property: $type
$$$
}'
# Check for the template usage of these properties
rg -A 5 'isQueued|estimatedCompletionDate|buildStartDate' --type html
Length of output: 10060
Script:
#!/bin/bash
# Search for the UpdatingResultComponent class implementation
rg -U "export class UpdatingResultComponent.*?\{.*?}" -A 50
# Also check for any interfaces or types that might define these properties
rg "interface.*?(Result|Timing|Queue)" -A 10
Length of output: 11955
Script:
#!/bin/bash
# Search for the component file directly
fd "updating-result.component.ts" --type f --exec cat {}
# Backup search with ripgrep
rg -l "UpdatingResultComponent" --type ts | xargs cat
Length of output: 223441
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (2)
3-3: LGTM: Appropriate imports added
The new imports align with the coding guidelines and support the enhanced build status update functionality:
- Using
ZonedDateTimeensures proper timezone handling - Using a dedicated DTO (
SubmissionProcessingDTO) follows the minimal DTO principle
Also applies to: 25-26
154-157: LGTM: Well-structured notification method
The new method follows good practices:
- Single responsibility principle
- Clear naming
- Appropriate access level
- Proper delegation to messaging service
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingSubmissionRepository.java (1)
162-174: Verify the impact on build status updates.
These new methods appear to be part of the build status update enhancement. Please ensure:
- The lighter fetch joins are sufficient for the build status update use case
- The ordering by ID (instead of submission date) is intentional for this use case
✅ Verification successful
Let me gather more information about the build status update context and fetch joins.
Let me check one more aspect to understand how these methods are used in the build status context.
Based on the gathered information, I can now provide the final response:
The new methods are safe for build status updates
The methods are used in ProgrammingSubmissionService for submission creation and retrieval, with two variants:
- A lighter version with basic fetch joins (participation only)
- An extended version with additional fetch joins (feedbacks and team students)
The ordering by ID is appropriate as it ensures consistent retrieval of the latest submission when multiple submissions exist for the same commit hash, as timestamps might be identical in such cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these methods are used in the context of build status updates
# and verify the consistency of ordering and fetch joins across similar use cases
# Check for usages of these methods in the codebase
echo "Searching for direct usages of the new methods..."
rg "findByParticipationIdAndCommitHashOrderByIdDesc|findFirstByParticipationIdAndCommitHashOrderByIdDesc"
# Check for similar patterns of submission retrieval
echo "Searching for similar submission retrieval patterns..."
ast-grep --pattern 'SELECT s FROM ProgrammingSubmission s WHERE s.participation.id = $_ AND s.commitHash = $_'
Length of output: 2422
Script:
#!/bin/bash
# Check the service methods using these repository methods
echo "Checking service methods context..."
rg -A 10 "findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents" src/main/java/de/tum/cit/aet/artemis/programming/service/
# Check for other similar queries with different fetch joins
echo "Checking other similar queries with fetch joins..."
ast-grep --pattern $'@Query("""
SELECT s
FROM ProgrammingSubmission s
LEFT JOIN FETCH $$$
WHERE $$$
""")'
# Look for build status related code
echo "Checking build status related code..."
rg -l "BuildStatus|build.*status" src/main/java/de/tum/cit/aet/artemis/programming/
Length of output: 5282
Script:
#!/bin/bash
# Check how these methods are used in build-related services
echo "Checking build-related service usage..."
rg -B 5 -A 10 "findFirstByParticipationIdAndCommitHash" src/main/java/de/tum/cit/aet/artemis/programming/service/localci/ src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/ src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/
# Check the ProgrammingSubmissionService for build status related code
echo "Checking submission service build status context..."
rg -B 5 -A 10 "BuildStatus|build.*status" src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingSubmissionService.java
# Check other similar queries in the repository
echo "Checking other submission queries..."
rg "@Query.*ProgrammingSubmission.*ORDER BY" src/main/java/de/tum/cit/aet/artemis/programming/repository/
Length of output: 782
src/main/webapp/i18n/de/editor.json (1)
34-36: LGTM! The translations are consistent and follow guidelines.
The new German translations for build queue status and ETA information:
- Follow the informal language requirement
- Maintain consistency with existing translations
- Align with the PR's objective of improving build status visibility
src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java (2)
32-32: LGTM: Imports follow best practices
The new imports are properly organized and follow Java best practices by avoiding star imports.
Also applies to: 35-35
201-211: Verify integration with client-side updates
The endpoint implementation aligns with the PR objectives for improving build status updates. However, let's verify the integration with client-side code that displays the queue status.
✅ Verification successful
Integration with client-side code is properly implemented
The endpoint is correctly integrated with the client-side code:
- The endpoint is consumed in
programming-submission.service.tsfor fetching queue duration estimations - It's part of a broader build queue management system with comprehensive client-side components:
BuildQueueComponentfor displaying queue statusBuildQueueServicefor managing queue-related operations- Integration with programming submission workflow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for corresponding client-side code that consumes this endpoint
# Look for API calls to this endpoint in frontend code
rg -t js -t ts "queued-jobs/queue-duration-estimation"
# Look for queue status display components
rg -t ts "QueuedStatus|BuildQueue" --type ts
Length of output: 13702
src/main/webapp/app/exercises/shared/result/updating-result.component.ts (2)
7-7: LGTM: New properties align with feature requirements
The new properties and imports are well-structured and follow Angular naming conventions:
showProgressBarfor UI controlisQueuedfor queue state trackingestimatedCompletionDateandbuildStartDatefor timing management
Also applies to: 38-38, 49-51
141-141: Verify websocket status update implementation
The PR objectives mention websocket notifications for status updates, but let's verify the complete implementation.
Also applies to: 179-179
✅ Verification successful
Websocket implementation is properly integrated
The code correctly implements websocket status updates through the ParticipationWebsocketService. The component:
- Injects and uses the websocket service for real-time updates
- Properly handles submission state changes via
updateSubmissionState - Updates build queue status and timing information
- Manages different states (queued, building, failed) appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for websocket status update implementations
rg -l "websocket.*status.*update" --type ts
# Search for related service implementations
ast-grep --pattern 'class $_ implements $_ {
$$$
updateStatus($_, $_) {
$$$
}
$$$
}'
Length of output: 268
Script:
#!/bin/bash
# Let's try a broader search for websocket-related code and status updates
rg -l "websocket" --type ts
# Search for the specific component and its dependencies
rg -l "updating-result.component" --type ts
# Look for the actual implementation of updateSubmissionState
ast-grep --pattern 'updateSubmissionState($_, $_) {
$$$
}'
# Search for any submission state update patterns
rg "submissionState.*update" --type ts
Length of output: 12954
Script:
#!/bin/bash
# Let's examine the updating-result.component.ts implementation
rg -A 10 "updateSubmissionState" src/main/webapp/app/exercises/shared/result/updating-result.component.ts
# Check for websocket service usage in the component
rg "websocket" src/main/webapp/app/exercises/shared/result/updating-result.component.ts
# Look for participation websocket service implementation
rg -A 10 "class ParticipationWebsocketService" src/main/webapp/app/overview/participation-websocket.service.ts
Length of output: 3019
src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java (2)
113-114: LGTM!
The initialization of new fields is consistent with the existing pattern and maintains proper encapsulation by using setter methods.
88-93: Verify database migration for new columns.
The addition of new columns requires a database migration script. Please ensure that:
- A migration script exists to add these columns
- Default values are handled for existing rows
- The migration has been tested in a staging environment
✅ Verification successful
Database migration is properly handled
The changes are properly managed through Liquibase migration:
- Migration script
20241101120000_changelog.xmladds both columns toprogramming_exercise_build_configtable - Default values (0) are correctly set for both columns
- The migration is included in the master changelog
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for database migration files related to the new columns
# Look for migration files that might contain the new column definitions
rg -l "build_duration_seconds|successful_build_count" --type sql
Length of output: 215
Script:
#!/bin/bash
# First, let's find all SQL migration files regardless of their location
fd -e sql
# Then, let's also check for Liquibase/Flyway migration files which might have different extensions
fd -e xml -e yaml -e yml | grep -i "migration\|changelog"
# Additionally, let's check for recent changes in migration-related files
git log -p --since="3 months ago" | grep -A 10 -B 10 "build_duration_seconds\|successful_build_count"
# Let's also check the entity class usage to understand if this is a new class or modification
rg -l "ProgrammingExerciseBuildConfig"
Length of output: 5328
src/test/javascript/spec/component/shared/updating-result.component.spec.ts (1)
7-12: LGTM: Well-organized imports
The new imports are properly grouped and aligned with the testing requirements for build timing and submission states.
src/main/webapp/app/exercises/shared/result/result.component.ts (2)
173-175: LGTM! Proper cleanup of interval.
The cleanup of estimatedDurationInterval prevents memory leaks and follows Angular best practices.
220-220: LGTM! Proper integration of queued state.
The addition of isQueued parameter to evaluateTemplateStatus is consistent with the existing pattern and properly integrates the new queued state handling.
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
67-70: LGTM! Consider adding documentation for clarity.
The constants follow the established naming and structural patterns. They align well with the PR's objective of improving build status updates via websocket communication.
Consider adding Javadoc comments to document the purpose and usage context of these constants, similar to other documented constants in this file. Example:
+ /**
+ * Constants for submission processing status updates via websockets.
+ * Used to differentiate between queued and processing states of build jobs.
+ */
public static final String SUBMISSION_PROCESSING = "/submissionProcessing";
public static final String SUBMISSION_PROCESSING_TOPIC = "/topic" + SUBMISSION_PROCESSING;Let's verify the consistent usage of these constants:
✅ Verification successful
Constants are properly used and integrated
The verification shows:
- The constants are correctly imported and used in
ProgrammingMessagingService - No hardcoded values found that should be using these constants
- Websocket configuration properly supports the topic prefix used in
SUBMISSION_PROCESSING_TOPIC
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the new constants in websocket/messaging related code
# Search for direct usage of the constants
echo "Checking direct usage of constants..."
rg "SUBMISSION_PROCESSING[^_]" --type java
# Search for hardcoded values that should use these constants
echo "Checking for potential hardcoded values..."
rg '"/submissionProcessing"' --type java
rg '"/topic/submissionProcessing"' --type java
# Search for related websocket configuration
echo "Checking websocket configuration..."
rg "configureMessageBroker|enableSimpleBroker|setApplicationDestinationPrefixes" --type java
Length of output: 1672
src/main/webapp/app/exercises/shared/result/result.utils.ts (3)
32-36: LGTM! Well-documented enum value addition.
The new IS_QUEUED status is well-placed and properly documented, clearly indicating its purpose for programming exercises.
158-158: LGTM! Backward-compatible parameter addition.
The new isQueued parameter is well-placed and maintains backward compatibility with its default value of false.
221-223: Verify handling of concurrent queue and build states.
While the implementation is correct, please verify the behavior when both isQueued and isBuilding are true. The current implementation will show the queued state, which seems logical, but it would be good to confirm this is the intended behavior.
Let's verify if this edge case is handled consistently across the codebase:
✅ Verification successful
Let me search for more context about the handling of queued and building states.
Let me try a different approach to find the evaluateTemplateStatus function implementation.
Let me try one more search with a different pattern to find the implementation.
The handling of concurrent queue and build states is implemented correctly
The implementation is consistent with the intended behavior. The code explicitly checks isQueued before isBuilding and returns ResultTemplateStatus.IS_QUEUED when a submission is queued, regardless of the build state. This is the correct approach because:
- The enum definition shows these are distinct states for programming exercises
- The queued state is logically checked first as it represents an earlier stage in the processing pipeline
- The implementation ensures a clear, deterministic state display to users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other places where isQueued and isBuilding are used together
# to ensure consistent handling of concurrent states
# Search for patterns where both flags might be used
rg -A 5 "isQueued.*isBuilding|isBuilding.*isQueued"
# Search for related test cases
rg -A 5 "test.*['\"]queue.*build|test.*['\"]build.*queue"
Length of output: 287030
Script:
#!/bin/bash
# Search for isQueued and isBuilding usage in result.utils.ts and related files
rg -A 5 "isQueued.*isBuilding|isBuilding.*isQueued" src/main/webapp/app/exercises/shared/result/
# Search for the evaluateTemplateStatus function implementation
ast-grep --pattern $'function evaluateTemplateStatus($$$) {
$$$
}'
Length of output: 1203
Script:
#!/bin/bash
# Search for the evaluateTemplateStatus function and its implementation
rg -A 30 "export function evaluateTemplateStatus" src/main/webapp/app/exercises/shared/result/result.utils.ts
Length of output: 111
Script:
#!/bin/bash
# Search for the evaluateTemplateStatus function with more context
rg -B 5 -A 30 "isQueued\)" src/main/webapp/app/exercises/shared/result/result.utils.ts
# Also search for any ResultTemplateStatus enum definition
rg -A 10 "enum ResultTemplateStatus" src/main/webapp/app/exercises/shared/result/
Length of output: 2795
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
396-397: LGTM: Comprehensive job timing tracking.
The implementation correctly captures both actual and estimated timing information, providing valuable data for future estimation improvements.
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (2)
98-100: LGTM: Field declarations follow best practices.
The new fields are appropriately declared as private and their types clearly indicate their purpose in managing build job queues.
132-133: LGTM: Proper initialization in BeforeEach.
The fields are correctly initialized in the setup method using the hazelcastInstance, ensuring clean state for each test.
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
97-98: LGTM: Constructor injection follows best practices.
The addition of SharedQueueManagementService as an Optional dependency is well-implemented using constructor injection, which is the recommended approach for dependency injection.
Also applies to: 103-104, 118-118
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java
Outdated
Show resolved
Hide resolved
.../java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java
Show resolved
Hide resolved
.../java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java
Show resolved
Hide resolved
...in/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
Show resolved
Hide resolved
...in/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
Outdated
Show resolved
Hide resolved
...in/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (5)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (5)
9-10: LGTM: Clean dependency injection and imports.
The new dependencies are properly injected via constructor injection, and the field is correctly marked as final. The imports are well-organized and follow Java conventions.
Also applies to: 28-28, 55-56, 58-59, 65-65
87-87: LGTM: Updated SubmissionDTO creation with processing state.
The modification correctly implements the new submission state tracking by explicitly setting isProcessing to false for new submissions, aligning with the PR's objective to differentiate between queued and processing states.
159-161: LGTM: Well-structured topic helper method.
The new helper method follows the established pattern for topic generation and maintains consistency with other similar methods in the class.
188-196: LGTM: Well-documented method signature.
The method's documentation clearly describes its purpose and parameters, making it easy to understand and maintain.
198-198: Verify repository method usage.
The repository method findWithProgrammingExerciseWithBuildConfigById eagerly loads the build config. Let's verify if this is necessary and if it could cause N+1 query issues.
✅ Verification successful
Let me gather more information to understand if the build config is actually used after being loaded.
Let me try one more search to find the query definition:
Based on the analysis of the code, I can now generate the final response:
The build config eager loading is necessary in this context
The repository method findWithProgrammingExerciseWithBuildConfigById is used in two critical flows:
- In
ProgrammingMessagingServicefor notifying users about submission processing, where the build config is needed for proper message handling - In
LocalCIResultProcessingServicefor processing build results, where the build config is explicitly used for:- Updating exercise build duration statistics
- Processing programming exercise results
- Handling build notifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if build config is actually used in the notification flow
# and look for similar repository methods to ensure consistent usage
# Check if build config is used in the notification flow
rg -A 5 "findWithProgrammingExerciseWithBuildConfigById" --type java
# Look for similar repository methods to compare usage patterns
ast-grep --pattern 'interface $_ extends $_ {
$$$
find$_WithBuildConfig$_($_) {
$$$
}
$$$
}'
Length of output: 3172
Script:
#!/bin/bash
# Check how the participation object is used after fetching with build config
rg -B 5 -A 15 "findWithProgrammingExerciseWithBuildConfigById" src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java
# Check the repository query to understand what's being loaded
rg "@Query.*findWithProgrammingExerciseWithBuildConfigById" --type java -A 5
Length of output: 1740
Script:
#!/bin/bash
# Search for the query definition in repository files
rg -l "findWithProgrammingExerciseWithBuildConfigById" --type java | xargs cat
# Alternative search for the query
fd -e java | xargs grep -l "findWithProgrammingExerciseWithBuildConfigById" | xargs grep -A 10 "findWithProgrammingExerciseWithBuildConfigById"
Length of output: 40468
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java
Show resolved
Hide resolved
WalkthroughThe pull request introduces significant modifications across multiple classes to enhance the management of build job timing information. Key changes include updates to constructors and methods in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmdsrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobQueueItem.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/buildagent/dto/JobTimingInfo.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.
🔧 eslint
src/main/webapp/app/entities/programming/programming-submission.model.tsOops! Something went wrong! :( ESLint: 9.14.0 TypeError: Error while loading rule '@typescript-eslint/no-unused-expressions': Cannot read properties of undefined (reading 'allowShortCircuit') Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (37)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (2)
334-335: Correct the misleading log message when agent capacity is exceededThe log message indicates that there are more agents available than expected. However, in this context, it's actually that there are more processing jobs than the capacity can handle. Updating the message will improve clarity.
Apply this diff to correct the log message:
- log.warn("There are more agents available than expected. This should not happen. Processing jobs: {}, Build agents: {}", processingJobs, buildAgentInformation); + log.warn("There are more processing jobs than the build agents' capacity. This should not happen. Processing jobs: {}, Build agents: {}", processingJobs, buildAgentInformation);
400-403: RenamegetBuildAgentsCapacity()to reflect its purposeThe method
getBuildAgentsCapacity()updates the internal state without returning a value. Renaming it toupdateBuildAgentsCapacity()orrefreshBuildAgentsCapacity()would enhance readability and convey its intent more clearly.Apply this diff to rename the method:
- private void getBuildAgentsCapacity() { + private void updateBuildAgentsCapacity() {Also, update all references to this method accordingly:
- this.getBuildAgentsCapacity(); + this.updateBuildAgentsCapacity();src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts (1)
215-227: Potential redundancy in resetting queue estimate timerIn
startQueueEstimateTimer, callingresetQueueEstimateTimerinside the timer's completion callback may be unnecessary since the timer completes after emitting. Consider reviewing whether this call is needed.If the intention is to ensure cleanup, and no action is needed, this can be left as is.
src/main/webapp/app/entities/programming/submission-processing-dto.ts (2)
3-9: Add JSDoc documentation for better code maintainability.Consider adding class and property documentation to improve code maintainability and clarity:
+/** + * Data Transfer Object for submission processing status and timing information. + * Used to communicate build job status updates between server and client. + */ export class SubmissionProcessingDTO { + /** ID of the exercise this submission belongs to */ public exerciseId?: number; + /** ID of the participation this submission is associated with */ public participationId?: number; + /** Git commit hash of the submission */ public commitHash?: string; + /** Estimated completion time for the build job */ public estimatedCompletionDate?: dayjs.Dayjs; + /** Actual start time of the build process */ public buildStartDate?: dayjs.Dayjs; }
3-9: Consider making critical fields required for type-safety.Based on the PR objectives, some fields might be essential for tracking build status. Consider making critical fields required by removing the optional modifier:
export class SubmissionProcessingDTO { - public exerciseId?: number; - public participationId?: number; + public exerciseId: number; + public participationId: number; public commitHash?: string; public estimatedCompletionDate?: dayjs.Dayjs; public buildStartDate?: dayjs.Dayjs; }src/main/webapp/app/exercises/shared/result/updating-result.component.html (1)
15-17: LGTM! Consider adding a tooltip for estimated completion time.The timing properties (
estimatedCompletionDate,buildStartDate) and progress bar enable the new estimation feature. This implementation aligns well with the PR objectives for providing better build status feedback.Consider enhancing the user experience by adding a tooltip that shows the exact estimated completion time when hovering over the progress bar.
<jhi-result id="result" [exercise]="exercise" [result]="result" [participation]="participation" [isBuilding]="isBuilding" [isQueued]="isQueued" [short]="short" [showUngradedResults]="showUngradedResults" [showBadge]="showBadge" [showIcon]="showIcon" [missingResultInfo]="missingResultInfo" [isInSidebarCard]="isInSidebarCard" [showCompletion]="showCompletion" [estimatedCompletionDate]="estimatedCompletionDate" [buildStartDate]="buildStartDate" [showProgressBar]="showProgressBar" + [tooltip]="estimatedCompletionDate | date:'medium'" />src/main/webapp/app/entities/programming/programming-submission.model.ts (2)
9-11: Add JSDoc documentation for new properties.The new properties would benefit from JSDoc documentation explaining their purpose and usage, especially since they're part of a public API.
Apply this diff:
+ /** Indicates whether the submission is currently being processed by the build system */ public isProcessing?: boolean; + /** The timestamp when the build process started */ public buildStartDate?: dayjs.Dayjs; + /** The estimated timestamp when the build process will complete */ public estimatedCompletionDate?: dayjs.Dayjs;
9-11: Consider grouping related properties together.The new timing-related properties (
buildStartDateandestimatedCompletionDate) should be grouped with other timing-related fields, while status-related properties (isProcessingandbuildFailed) should be grouped together.Consider reordering the properties like this:
public commitHash?: string; + /** Indicates whether the submission is currently being processed by the build system */ + public isProcessing?: boolean; public buildFailed?: boolean; public buildArtifact?: boolean; // whether the result includes a build artifact or not - public isProcessing?: boolean; + /** The timestamp when the build process started */ public buildStartDate?: dayjs.Dayjs; + /** The estimated timestamp when the build process will complete */ public estimatedCompletionDate?: dayjs.Dayjs;src/test/javascript/spec/helpers/mocks/service/mock-programming-submission.service.ts (1)
18-18: Consider making the mock more configurable.While the current implementation is valid, consider making it configurable to support different test scenarios. This would allow tests to verify behavior for both local and non-local CI profiles.
- getIsLocalCIProfile = () => false; + private isLocalCI = false; + getIsLocalCIProfile = () => this.isLocalCI; + setIsLocalCIProfile = (value: boolean) => { this.isLocalCI = value; };src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseBuildConfigRepository.java (1)
21-23: LGTM! Consider adding Javadoc for better documentation.The new method follows good practices by reusing existing functionality and providing a convenient way to handle missing configurations with exceptions.
Add Javadoc to document the method's behavior and the exception thrown:
+ /** + * Retrieves a programming exercise build configuration by its exercise ID. + * + * @param programmingExerciseId the ID of the programming exercise + * @return the build configuration + * @throws EntityNotFoundException if no configuration exists for the given exercise ID + */ default ProgrammingExerciseBuildConfig findByProgrammingExerciseIdElseThrow(Long programmingExerciseId) { return getValueElseThrow(findByProgrammingExerciseId(programmingExerciseId), programmingExerciseId); }src/main/java/de/tum/cit/aet/artemis/exercise/dto/SubmissionDTO.java (1)
18-19: LGTM! Consider documenting the new fields.The new fields align well with the DTO guidelines and use appropriate types. Consider adding Javadoc descriptions for the new fields to clarify their purpose and usage.
Add field documentation:
/** * Indicates if the submission is currently being processed */ boolean isProcessing, /** * The date when the build process started */ ZonedDateTime buildStartDate, /** * The estimated completion date for the build process */ ZonedDateTime estimatedCompletionDatesrc/main/java/de/tum/cit/aet/artemis/programming/dto/ResultDTO.java (2)
66-66: Consider using Optional or builder pattern instead of null parameters.While the change supports the new build status tracking feature, passing explicit null values for
buildStartDateandestimatedCompletionDatecould be improved. Consider:
- Using
Optional<ZonedDateTime>for the date parameters- Implementing a builder pattern for
SubmissionDTO- Creating separate factory methods for different states
Example approach:
// Option 1: Using Optional SubmissionDTO.of(result.getSubmission(), false, Optional.empty(), Optional.empty()); // Option 2: Using builder pattern SubmissionDTO.builder() .submission(result.getSubmission()) .isProcessing(false) .build(); // Option 3: Separate factory methods SubmissionDTO.ofUnprocessed(result.getSubmission());
Line range hint
17-19: Consider addressing the TODO comment as part of this PR.The TODO suggests moving result string calculations to the server-side, which aligns well with the current PR's focus on improving build status updates. This change would:
- Ensure consistent result formatting across different clients
- Simplify client-side logic
- Better align with server-side processing principles
Would you like me to help implement this server-side calculation feature or create a GitHub issue to track this enhancement?
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (2)
56-60: Update JavaDoc to include the new parameter.The constructor's JavaDoc comment is missing documentation for the
programmingMessagingServiceparameter.Apply this diff to update the documentation:
/** * Instantiates a new Local ci queue websocket service. * * @param hazelcastInstance the hazelcast instance * @param localCIWebsocketMessagingService the local ci build queue websocket service * @param sharedQueueManagementService the local ci shared build job queue service + * @param programmingMessagingService the programming messaging service */
154-157: Add null checks for date parameters.While the implementation is clean, consider adding null checks for
buildStartDateandestimatedCompletionDateto prevent potential NullPointerException when creating the DTO.Consider this improvement:
private void notifyUserAboutBuildProcessing(long exerciseId, long participationId, String commitHash, ZonedDateTime buildStartDate, ZonedDateTime estimatedCompletionDate) { + if (buildStartDate == null || estimatedCompletionDate == null) { + log.warn("Build timing information is missing for exercise {} and participation {}", exerciseId, participationId); + return; + } var submissionProcessingDTO = new SubmissionProcessingDTO(exerciseId, participationId, commitHash, buildStartDate, estimatedCompletionDate); programmingMessagingService.notifyUserAboutSubmissionProcessing(submissionProcessingDTO, exerciseId, participationId); }src/main/webapp/app/exercises/shared/result/result.component.html (1)
16-37: Enhance accessibility and maintainability of the progress bar.While the implementation provides good visual feedback, consider the following improvements:
- Add ARIA attributes for better accessibility:
- <div class="progress position-relative" role="progressbar" [style.min-width.px]="200" [style.height.px]="30"> + <div + class="progress position-relative" + role="progressbar" + [attr.aria-valuenow]="progressBarValue" + aria-valuemin="0" + aria-valuemax="100" + [attr.aria-label]="'artemisApp.editor.buildProgress' | artemisTranslate" + [style.min-width.px]="200" + [style.height.px]="30">
- Move inline styles to CSS:
- <div class="position-absolute fw-bold" style="top: 6px; left: 70px" [style.font-size.px]="14.4"> + <div class="progress-text fw-bold">Add to your CSS:
.progress-text { top: 6px; left: 70px; font-size: 14.4px; }
- Consider extracting the estimated time display into a reusable template to avoid duplication between progress bar and fallback views.
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingSubmissionRepository.java (1)
172-174: Add Javadoc documentation.The implementation looks good, but please add Javadoc to:
- Document the method's purpose
- Describe parameters
- Specify return value semantics (especially the null case)
Example:
/** * Finds the first programming submission for a given participation and commit hash. * * @param participationId the ID of the participation * @param commitHash the commit hash to search for * @return the most recent submission matching the criteria, or null if none found */src/main/webapp/i18n/de/editor.json (1)
34-36: LGTM with a minor suggestion for clarity!The translations are accurate and correctly use informal language as per guidelines. However, consider making the ETA explanation more user-friendly:
- "etaInfo": "Geschätzte Zeit bis zum Abschluss des Builds. Dies ist eine Schätzung und kann variieren.", + "etaInfo": "Geschätzte Zeit bis zum Abschluss des Builds. Diese Schätzung kann je nach Auslastung variieren.",This change makes it clearer why the estimate might vary.
src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java (1)
201-203: Consider adding rate limitingGiven that this endpoint is accessible to students and could be frequently called, consider adding rate limiting to prevent potential abuse.
src/main/webapp/app/exercises/shared/result/updating-result.component.ts (1)
141-141: Consider enhancing error handling for timing updatesWhile the state management is solid, consider adding error handling for timing updates to gracefully handle potential issues with the build timing information.
private updateSubmissionState(submissionState: ProgrammingSubmissionState, buildTimingInfo?: BuildTimingInfo) { this.isQueued = submissionState === ProgrammingSubmissionState.IS_QUEUED; this.isBuilding = submissionState === ProgrammingSubmissionState.IS_BUILDING_PENDING_SUBMISSION; - if (this.submissionService.getIsLocalCIProfile()) { - this.updateBuildTimingInfo(submissionState, buildTimingInfo); + try { + if (this.submissionService.getIsLocalCIProfile()) { + this.updateBuildTimingInfo(submissionState, buildTimingInfo); + } + } catch (error) { + console.error('Failed to update build timing information:', error); + // Reset timing information to avoid showing stale data + this.estimatedCompletionDate = undefined; + this.buildStartDate = undefined; }Also applies to: 178-184
src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java (2)
88-93: Add column constraints and documentation for new fields.The new fields would benefit from:
- Explicit column constraints for data integrity
- Documentation explaining their purpose and usage
+ /** + * The average duration of successful builds in seconds. + * Used for estimating completion times of queued builds. + */ @Column(name = "build_duration_seconds", nullable = false) private long buildDurationSeconds = 0; + /** + * The total number of successful builds. + * Used in conjunction with buildDurationSeconds for calculating average build times. + */ @Column(name = "successful_build_count", nullable = false) private long successfulBuildCount = 0;
Based on the gathered information, I can now provide a final response:
Database migration exists but build status update integration is missing
The new fields are properly added to the database schema through the migration changelog
20241101120000_changelog.xml, but there's no evidence of code that updates these metrics during build completion. You need to:
- Add logic to update
buildDurationSecondsandsuccessfulBuildCountin the build completion handlers- Implement the duration estimation algorithm using these fields
- Consider adding the update logic in one of these services:
BuildJobManagementServiceBuildJobExecutionService- Your chosen CI implementation service (Jenkins/GitLab/Aeolus)
🔗 Analysis chain
Line range hint
88-318: Verify integration with build queue duration estimation system.The new fields support the PR's objective of providing build duration estimates. Please ensure:
- These fields are properly updated when builds complete
- The values are used correctly in the estimation algorithm
- The fields are included in database migration scripts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check integration points for build metrics # Check for build completion handlers rg -A 5 "buildDurationSeconds|successfulBuildCount" --type java # Check for database migration scripts fd -e sql | xargs grep -l "build_duration_seconds\|successful_build_count"Length of output: 3516
Script:
#!/bin/bash # Let's search for build-related services and handlers that might update these metrics # Search for build completion or status update handlers ast-grep --pattern 'class $_ implements BuildStatusUpdateHandler { $$$ }' # Search for build service implementations ast-grep --pattern 'class $_ { $$$ updateBuildStatus($$$) { $$$ } $$$ }' # Search for database migration files specifically fd -e sql -e xml -e yaml -e yml | grep -i "changelog\|migration\|liquibase"Length of output: 3099
Script:
#!/bin/bash # Let's check the most recent changelog files and build-related services # Check recent changelog files for our columns cat src/main/resources/config/liquibase/changelog/20241112123600_changelog.xml src/main/resources/config/liquibase/changelog/20241101120000_changelog.xml # Search for build service classes and their usage of these fields rg -g "*.java" -B 3 -A 3 "class.*Build.*Service" # Search for build status update implementations rg -g "*.java" -B 3 -A 3 "updateBuildStatus|onBuildCompleted"Length of output: 18045
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIServiceTest.java (3)
Line range hint
73-89: Consider adding test cases for queue-based timing scenariosThe test verifies basic build status transitions but doesn't cover the new queue duration estimation system mentioned in the PR objectives.
Consider adding test cases for:
- Multiple queued jobs affecting estimation
- Build agent availability impact
- Queue position changes
Example:
@Test void testQueueBasedTimingEstimation() { // Setup multiple jobs BuildJobQueueItem job1 = createBuildJob("1", estimatedDuration1); BuildJobQueueItem job2 = createBuildJob("2", estimatedDuration2); // Add jobs to queue queuedJobs.add(job1); queuedJobs.add(job2); // Verify that job2's estimated completion considers job1's duration assertThat(job2.getJobTimingInfo().estimatedCompletionDate()) .isAfter(job1.getJobTimingInfo().estimatedCompletionDate()); }
Line range hint
1-89: Add test class documentationThe test class lacks documentation explaining its purpose and the scenarios it covers, particularly regarding the new timing estimation features.
Add class-level documentation:
+/** + * Tests for LocalCIService focusing on: + * - Build status transitions + * - Job timing estimation + * - Queue management + * - Build agent interaction + */ class LocalCIServiceTest extends AbstractProgrammingIntegrationLocalCILocalVCTest {
Line range hint
1-89: Missing test coverage for key PR featuresThe test class doesn't cover several key features mentioned in the PR objectives:
- Build queue duration estimation system
- Server-client communication for job status updates
- Progress bar animation triggers
Would you like me to help create additional test methods to cover these scenarios? This could include:
- Integration tests for queue estimation accuracy
- Tests for status update message sequences
- Tests verifying correct client notification timing
src/test/javascript/spec/component/exercises/shared/result.spec.ts (1)
227-233: Consider adding tests for build status user experienceGiven the PR's focus on improving build status updates for users, consider adding test cases that validate:
- Status transitions (queued → processing)
- Progress bar animation triggers
- Duration estimation accuracy
Example test case:
it('should update UI elements when build status changes', () => { // Setup initial queued state component.buildStatus = 'queued'; component.estimatedCompletionDate = dayjs().add(20, 'seconds'); fixture.detectChanges(); // Verify queued state UI expect(component.getStatusText()).toBe('Queued'); expect(component.showProgressBar).toBeTrue(); // Transition to processing component.buildStatus = 'processing'; fixture.detectChanges(); // Verify processing state UI expect(component.getStatusText()).toBe('Processing'); expect(component.showProgressAnimation).toBeTrue(); });src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (1)
199-214: Consider optimizing repository query and improving error handlingThe method could benefit from some improvements:
- The repository query might be loading more data than necessary with the eager loading of build config.
- The error handling for the repository query could be more specific than just throwing a NoSuchElementException.
- The method could be split into smaller methods for better maintainability.
Consider refactoring like this:
public void notifyUserAboutSubmissionProcessing(SubmissionProcessingDTO submission, long exerciseId, long participationId) { - Participation participation = participationRepository.findWithProgrammingExerciseWithBuildConfigById(participationId).orElseThrow(); + Participation participation = participationRepository.findWithProgrammingExerciseWithBuildConfigById(participationId) + .orElseThrow(() -> new EntityNotFoundException("Participation not found with ID: " + participationId)); + + notifyRelevantUsers(participation, submission); +} + +private void notifyRelevantUsers(Participation participation, SubmissionProcessingDTO submission) { if (participation instanceof StudentParticipation studentParticipation) { - if (studentParticipation.getParticipant() instanceof Team team) { - // eager load the team with students so their information can be used for the messages below - studentParticipation.setParticipant(teamRepository.findWithStudentsByIdElseThrow(team.getId())); - } - studentParticipation.getStudents().forEach(user -> websocketMessagingService.sendMessageToUser(user.getLogin(), SUBMISSION_PROCESSING_TOPIC, submission)); + notifyStudents(studentParticipation, submission); } // send an update to tutors, editors and instructors about submissions for template and solution participations if (!(participation instanceof StudentParticipation)) { - var topicDestination = getSubmissionProcessingTopicForTAAndAbove(exerciseId); + var topicDestination = getSubmissionProcessingTopicForTAAndAbove(participation.getExercise().getId()); websocketMessagingService.sendMessage(topicDestination, submission); } } + +private void notifyStudents(StudentParticipation studentParticipation, SubmissionProcessingDTO submission) { + if (studentParticipation.getParticipant() instanceof Team team) { + // eager load the team with students so their information can be used for the messages below + studentParticipation.setParticipant(teamRepository.findWithStudentsByIdElseThrow(team.getId())); + } + studentParticipation.getStudents() + .forEach(user -> websocketMessagingService.sendMessageToUser( + user.getLogin(), + SUBMISSION_PROCESSING_TOPIC, + submission + )); +}src/main/webapp/app/exercises/shared/result/result.component.ts (1)
Line range hint
188-213: Consider refactoring interval setup logicWhile the logic is correct, consider these improvements for better maintainability:
- Extract interval setup to a separate method
- Define interval timing as a constant
- Consider using RxJS timer for more idiomatic Angular code
+ private readonly PROGRESS_UPDATE_INTERVAL = 1000; // 1 second + + private setupProgressInterval(): void { + if (!this.estimatedCompletionDate) return; + + clearInterval(this.estimatedDurationInterval); + this.estimatedDurationInterval = setInterval(() => { + this.estimatedRemaining = Math.max(0, dayjs(this.estimatedCompletionDate).diff(dayjs(), 'seconds')); + const estimatedDuration = dayjs(this.estimatedCompletionDate).diff(dayjs(this.buildStartDate), 'seconds'); + this.progressBarValue = Math.round((1 - this.estimatedRemaining / estimatedDuration) * 100); + + if (this.estimatedRemaining <= 0) { + clearInterval(this.estimatedDurationInterval); + } + }, this.PROGRESS_UPDATE_INTERVAL); + } ngOnChanges(changes: SimpleChanges) { // ... existing code ... - if (changes.estimatedCompletionDate && this.estimatedCompletionDate) { - clearInterval(this.estimatedDurationInterval); - this.estimatedDurationInterval = setInterval(() => { - this.estimatedRemaining = Math.max(0, dayjs(this.estimatedCompletionDate).diff(dayjs(), 'seconds')); - const estimatedDuration = dayjs(this.estimatedCompletionDate).diff(dayjs(this.buildStartDate), 'seconds'); - this.progressBarValue = Math.round((1 - this.estimatedRemaining / estimatedDuration) * 100); - if (this.estimatedRemaining <= 0) { - clearInterval(this.estimatedDurationInterval); - } - }, 1000); // 1 second - } + if (changes.estimatedCompletionDate && this.estimatedCompletionDate) { + this.setupProgressInterval(); + } }src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java (1)
197-197: Consider using a builder pattern for JobTimingInfoThe current constructor call with multiple null parameters reduces readability and maintainability. Consider using a builder pattern or factory methods to make the code more expressive and safer.
Example improvement:
JobTimingInfo jobTimingInfo = JobTimingInfo.builder() .submissionDate(submissionDate) .estimatedDuration(estimatedDuration) .build();This would require updating the
JobTimingInfoclass to implement the builder pattern.src/main/webapp/app/exercises/shared/result/result.utils.ts (2)
158-158: Consider refactoring function parameters using a configuration object.The function now has 6 parameters, which could make it harder to maintain and use. Consider refactoring to use a configuration object pattern for better maintainability.
-export const evaluateTemplateStatus = ( - exercise: Exercise | undefined, - participation: Participation | undefined, - result: Result | undefined, - isBuilding: boolean, - missingResultInfo = MissingResultInformation.NONE, - isQueued = false, -): ResultTemplateStatus => { +interface TemplateStatusConfig { + exercise?: Exercise; + participation?: Participation; + result?: Result; + isBuilding: boolean; + missingResultInfo?: MissingResultInformation; + isQueued?: boolean; +} + +export const evaluateTemplateStatus = ({ + exercise, + participation, + result, + isBuilding, + missingResultInfo = MissingResultInformation.NONE, + isQueued = false, +}: TemplateStatusConfig): ResultTemplateStatus => {
221-223: Consider simplifying the conditional logic.While the logic is correct, the nested if-else structure could be simplified using early returns for better readability.
- if (isQueued) { - return ResultTemplateStatus.IS_QUEUED; - } else if (isBuilding) { - return ResultTemplateStatus.IS_BUILDING; + if (isQueued) return ResultTemplateStatus.IS_QUEUED; + if (isBuilding) return ResultTemplateStatus.IS_BUILDING; + if (isAIResultAndIsBeingProcessed(result)) return ResultTemplateStatus.IS_GENERATING_FEEDBACK;src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
376-408: Test implementation is solid but could benefit from better documentation and naming.The test effectively verifies the queue duration estimation feature with comprehensive scenarios. However, consider these improvements:
- Rename the test method to better describe what it's testing (e.g.,
testBuildJobQueueDurationEstimation)- Add Javadoc to document the test's purpose and scenarios
- Extract magic numbers (24, 48) into named constants to improve readability
+ /** Duration in seconds for each build job */ + private static final int BUILD_JOB_DURATION = 24; + /** Expected total queue duration in seconds */ + private static final int EXPECTED_QUEUE_DURATION = 48; + - void testBuildJob() throws Exception { + /** + * Tests the build job queue duration estimation when multiple jobs are queued + * and being processed. Verifies that the estimation takes into account: + * - Currently processing jobs + * - Jobs ahead in the queue + * - Individual job duration estimates + */ + void testBuildJobQueueDurationEstimation() throws Exception { var now = ZonedDateTime.now(); - JobTimingInfo jobTimingInfo1 = new JobTimingInfo(now, now, null, now.plusSeconds(24), 24); + JobTimingInfo jobTimingInfo1 = new JobTimingInfo(now, now, null, now.plusSeconds(BUILD_JOB_DURATION), BUILD_JOB_DURATION); // ... similar changes for other JobTimingInfo instances ... var queueDurationEstimation = sharedQueueManagementService.getBuildJobEstimatedQueueDuration(job4.participationId()); - assertThat(queueDurationEstimation).isCloseTo(now.plusSeconds(48), within(1, ChronoUnit.SECONDS)); + assertThat(queueDurationEstimation).isCloseTo(now.plusSeconds(EXPECTED_QUEUE_DURATION), within(1, ChronoUnit.SECONDS));src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (2)
396-397: Improve JobTimingInfo parameter organizationThe current constructor call mixes actual and estimated timing parameters, making it harder to maintain and more prone to errors. Consider restructuring JobTimingInfo to group actual and estimated times.
-JobTimingInfo jobTimingInfo = new JobTimingInfo(buildJob.jobTimingInfo().submissionDate(), - buildJob.jobTimingInfo().buildStartDate(), - ZonedDateTime.now(), - buildJob.jobTimingInfo().estimatedCompletionDate(), - buildJob.jobTimingInfo().estimatedDuration()); +// Suggest refactoring JobTimingInfo to use a builder pattern: +JobTimingInfo jobTimingInfo = JobTimingInfo.builder() + .actualTimes() + .submissionDate(buildJob.jobTimingInfo().submissionDate()) + .buildStartDate(buildJob.jobTimingInfo().buildStartDate()) + .completionDate(ZonedDateTime.now()) + .estimatedTimes() + .completionDate(buildJob.jobTimingInfo().estimatedCompletionDate()) + .duration(buildJob.jobTimingInfo().estimatedDuration()) + .build();
Line range hint
1-585: Consider extracting timing estimation logic to a separate serviceThe
SharedQueueProcessingServicehandles both job processing and timing estimation logic. To improve maintainability and follow the Single Responsibility Principle, consider extracting the timing estimation logic into a dedicatedBuildJobTimingService.This would:
- Make the timing logic more maintainable and testable
- Allow for more sophisticated estimation algorithms
- Reduce the complexity of the SharedQueueProcessingService
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (1)
517-544: Consider enhancing test method documentation and assertionsWhile the test logic is sound, it could benefit from:
- A descriptive Javadoc explaining the test's purpose and expectations
- More granular assertions for timing calculations
- Cleanup of shared resources after test completion
Consider applying this enhancement:
+ /** + * Tests the build job timing information calculation and verification. + * Verifies: + * 1. Estimated duration calculation (build duration + 20% buffer) + * 2. Estimated completion date calculation + * 3. Proper timing updates during job state transitions + */ @Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") void testBuildJobTimingInfo() { // Pause build agent processing sharedQueueProcessingService.removeListenerAndCancelScheduledFuture(); ProgrammingExerciseBuildConfig buildConfig = programmingExercise.getBuildConfig(); buildConfig.setBuildDurationSeconds(20); programmingExerciseBuildConfigRepository.save(buildConfig); ProgrammingExerciseStudentParticipation studentParticipation = localVCLocalCITestService.createParticipation(programmingExercise, student1Login); localVCServletService.processNewPush(commitHash, studentAssignmentRepository.originGit.getRepository()); await().until(() -> queuedJobs.stream().anyMatch(buildJobQueueItem -> buildJobQueueItem.buildConfig().commitHashToBuild().equals(commitHash) && buildJobQueueItem.participationId() == studentParticipation.getId())); BuildJobQueueItem item = queuedJobs.stream().filter(i -> i.buildConfig().commitHashToBuild().equals(commitHash) && i.participationId() == studentParticipation.getId()) .findFirst().orElseThrow(); + // Verify that estimated duration includes 20% buffer (20 seconds + 20% = 24 seconds) assertThat(item.jobTimingInfo().estimatedDuration()).isEqualTo(24); sharedQueueProcessingService.init(); await().until(() -> processingJobs.values().stream().anyMatch(buildJobQueueItem -> buildJobQueueItem.buildConfig().commitHashToBuild().equals(commitHash) && buildJobQueueItem.participationId() == studentParticipation.getId())); item = processingJobs.values().stream().filter(i -> i.buildConfig().commitHashToBuild().equals(commitHash) && i.participationId() == studentParticipation.getId()) .findFirst().orElseThrow(); + // Verify timing calculations remain consistent after state transition assertThat(item.jobTimingInfo().estimatedDuration()).isEqualTo(24); assertThat(item.jobTimingInfo().estimatedCompletionDate()).isCloseTo(item.jobTimingInfo().buildStartDate().plusSeconds(24), within(500, ChronoUnit.MILLIS)); + + // Cleanup + try { + sharedQueueProcessingService.removeListenerAndCancelScheduledFuture(); + } finally { + sharedQueueProcessingService.init(); + } }src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (2)
217-243: Consider extracting build timing logic to a separate method.The method
getLatestPendingSubmissionhas grown in complexity with the addition of build timing information retrieval. Consider extracting the build timing logic (lines 229-239) into a separate private method for better readability and maintainability.public ResponseEntity<SubmissionDTO> getLatestPendingSubmission(@PathVariable Long participationId, @RequestParam(defaultValue = "false") boolean lastGraded) { Optional<ProgrammingSubmission> submissionOpt; try { submissionOpt = submissionService.getLatestPendingSubmission(participationId, lastGraded); } catch (IllegalArgumentException ex) { throw new EntityNotFoundException("participation", participationId); } if (submissionOpt.isEmpty()) { return ResponseEntity.ok(null); } ProgrammingSubmission programmingSubmission = submissionOpt.get(); - boolean isSubmissionProcessing = false; - ZonedDateTime buildStartDate = null; - ZonedDateTime estimatedCompletionDate = null; - if (sharedQueueManagementService.isPresent()) { - var buildTimingInfo = sharedQueueManagementService.get().isSubmissionProcessing(participationId, programmingSubmission.getCommitHash()); - isSubmissionProcessing = buildTimingInfo != null; - if (isSubmissionProcessing) { - buildStartDate = buildTimingInfo.buildStartDate(); - estimatedCompletionDate = buildTimingInfo.estimatedCompletionDate(); - } - } + var buildTiming = getBuildTiming(participationId, programmingSubmission.getCommitHash()); // Remove participation, is not needed in the response. programmingSubmission.setParticipation(null); - var submissionDTO = SubmissionDTO.of(programmingSubmission, isSubmissionProcessing, buildStartDate, estimatedCompletionDate); + var submissionDTO = SubmissionDTO.of(programmingSubmission, buildTiming.isProcessing(), buildTiming.startDate(), buildTiming.estimatedCompletion()); return ResponseEntity.ok(submissionDTO); } +private record BuildTiming(boolean isProcessing, ZonedDateTime startDate, ZonedDateTime estimatedCompletion) {} + +private BuildTiming getBuildTiming(Long participationId, String commitHash) { + if (sharedQueueManagementService.isPresent()) { + var buildTimingInfo = sharedQueueManagementService.get().isSubmissionProcessing(participationId, commitHash); + if (buildTimingInfo != null) { + return new BuildTiming(true, buildTimingInfo.buildStartDate(), buildTimingInfo.estimatedCompletionDate()); + } + } + return new BuildTiming(false, null, null); +}
232-234: Consider renaming the method for clarity.The method name
isSubmissionProcessingreturns more than just a boolean status. Consider renaming it to better reflect its purpose, such asgetSubmissionBuildStatusorgetSubmissionProcessingInfo.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241101120000_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (37)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobQueueItem.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/JobTimingInfo.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/dto/SubmissionDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/dto/ResultDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/dto/SubmissionProcessingDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseBuildConfigRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingSubmissionRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java(2 hunks)src/main/webapp/app/entities/programming/programming-submission.model.ts(1 hunks)src/main/webapp/app/entities/programming/submission-processing-dto.ts(1 hunks)src/main/webapp/app/exercises/programming/participate/code-editor-student-container.component.html(1 hunks)src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts(16 hunks)src/main/webapp/app/exercises/shared/result/result.component.html(1 hunks)src/main/webapp/app/exercises/shared/result/result.component.ts(6 hunks)src/main/webapp/app/exercises/shared/result/result.utils.ts(3 hunks)src/main/webapp/app/exercises/shared/result/updating-result.component.html(1 hunks)src/main/webapp/app/exercises/shared/result/updating-result.component.ts(5 hunks)src/main/webapp/app/overview/submission-result-status.module.ts(1 hunks)src/main/webapp/i18n/de/editor.json(1 hunks)src/main/webapp/i18n/en/editor.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java(6 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(5 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIServiceTest.java(1 hunks)src/test/javascript/spec/component/exercises/shared/result.spec.ts(1 hunks)src/test/javascript/spec/component/shared/updating-result.component.spec.ts(6 hunks)src/test/javascript/spec/helpers/mocks/service/mock-programming-submission.service.ts(2 hunks)src/test/javascript/spec/service/programming-submission.service.spec.ts(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/programming/dto/SubmissionProcessingDTO.java
🧰 Additional context used
📓 Path-based instructions (35)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobQueueItem.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/JobTimingInfo.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/exercise/dto/SubmissionDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/dto/ResultDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseBuildConfigRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingSubmissionRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/entities/programming/programming-submission.model.ts (1)
src/main/webapp/app/entities/programming/submission-processing-dto.ts (1)
src/main/webapp/app/exercises/programming/participate/code-editor-student-container.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts (1)
src/main/webapp/app/exercises/shared/result/result.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exercises/shared/result/result.component.ts (1)
src/main/webapp/app/exercises/shared/result/result.utils.ts (1)
src/main/webapp/app/exercises/shared/result/updating-result.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exercises/shared/result/updating-result.component.ts (1)
src/main/webapp/app/overview/submission-result-status.module.ts (1)
src/main/webapp/i18n/de/editor.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/javascript/spec/component/exercises/shared/result.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/shared/updating-result.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/helpers/mocks/service/mock-programming-submission.service.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/service/programming-submission.service.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🪛 Biome
src/test/javascript/spec/service/programming-submission.service.spec.ts
[error] 151-151: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 170-170: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (68)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (2)
170-175: Verify the necessity of setting both Exercise and ProgrammingExercise in participation
At lines 173 and 174, both setExercise(exercise) and setProgrammingExercise(exercise) are called on programmingExerciseParticipation. Please verify if setting both is necessary or if one is redundant. Streamlining this could improve code clarity.
269-269: Good practice: Changed saveFinishedBuildJob method visibility to private
Changing the saveFinishedBuildJob method from public to private enhances encapsulation and adheres to the principle of least privilege.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (1)
346-352:
Fix incorrect use of getLast() method on a List
The List interface does not have a getLast() method. To retrieve the last element, use get(size() - 1) instead.
Apply this diff to fix the issue:
- return participationBuildJobIds.getLast();
+ return participationBuildJobIds.get(participationBuildJobIds.size() - 1);Likely invalid or redundant comment.
src/test/javascript/spec/service/programming-submission.service.spec.ts (1)
445-458: Ensure websocket unsubscription logic is correct
In the test it('should only unsubscribe if no other participations use the topic with localci', ...), verify that the unsubscription logic accurately reflects the intended behavior when using the local CI profile. This ensures that resources are properly cleaned up without affecting other participations.
src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts (17)
16-19: Imports are correctly added and follow project conventions
The newly added imports are necessary and adhere to the project's import structure.
28-29: Consistent use of enum values in ProgrammingSubmissionState
The addition of IS_QUEUED maintains consistency with existing enum values.
32-37: Extended ProgrammingSubmissionStateObj with buildTimingInfo
The addition of the optional buildTimingInfo property is appropriate and follows the project's naming conventions.
39-42: Definition of BuildTimingInfo type
The new type BuildTimingInfo is well-defined with appropriately typed properties.
77-78: Addition of SUBMISSION_PROCESSING_TEMPLATE_TOPIC
The new topic for submission processing is correctly added and follows naming conventions.
82-83: Initialization of submissionProcessingTopicsSubscribed
The map for tracking processing topics is appropriately initialized.
97-98: Addition of queueEstimateTimerSubscriptions
The subscription management for queue estimate timers is correctly implemented.
101-102: Initialization of currentExpectedQueueEstimate
The current expected queue estimate is correctly set based on the default value.
103-104: Management of processing cache and profile flag
The startedProcessingCache and isLocalCIProfile variables are appropriately declared and initialized.
113-116: Proper subscription to profile service
The constructor correctly subscribes to the ProfileService to determine the active profiles.
121-123: Ensuring all subscriptions are unsubscribed in ngOnDestroy
The added unsubscription for queueEstimateTimerSubscriptions and submissionProcessingTopicsSubscribed helps prevent memory leaks.
856-857: Unsubscribe from queueEstimateTimerSubscriptions in unsubscribeAllWebsocketTopics
The addition ensures that all timer subscriptions are properly unsubscribed to prevent memory leaks.
884-894: Unsubscribe from submission processing topics in unsubscribeForLatestSubmissionOfParticipation
The implementation correctly handles unsubscription logic for processing topics.
900-908: Getter and setter for isLocalCIProfile
The addition of setLocalCIProfile and getIsLocalCIProfile maintains encapsulation and follows best practices.
706-708: Setup websocket subscription conditionally based on profile
The code appropriately sets up the submission processing websocket subscription only when isLocalCIProfile is true.
275-285: Ensure correct emission of queued submission and timer initiation
The logic for emitting a queued submission and starting the queue estimate timer appears sound.
268-291: Proper handling of submission processing state
The code correctly checks the processing state of the submission and handles timing information accordingly.
src/main/webapp/app/entities/programming/submission-processing-dto.ts (1)
1-1: LGTM! Import statement follows best practices.
The ESM import of dayjs is correctly specified using single quotes, adhering to Angular style guidelines.
src/main/webapp/app/exercises/shared/result/updating-result.component.html (1)
7-7: LGTM! Queue status property addition aligns with PR objectives.
The [isQueued] input property enables proper differentiation between queued and processing states, which directly addresses the PR's goal of providing clearer status updates to users.
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/JobTimingInfo.java (2)
13-14: Implementation follows best practices
The implementation:
- Uses Java records appropriately for DTOs
- Correctly implements Serializable for distributed systems
- Uses ZonedDateTime for proper timezone handling
- Uses primitive long for duration (avoiding unnecessary boxing)
- Has proper JSON annotations for serialization control
13-14: Consider the impact on serialized data and provide migration strategy
The addition of new fields to this shared data structure could impact existing serialized data in Hazelcast/Redis. While the @JsonIgnoreProperties(ignoreUnknown = true) helps with backward compatibility, a clear migration strategy should be documented.
Let's verify the usage of this DTO in the codebase:
src/main/webapp/app/overview/submission-result-status.module.ts (2)
7-7: LGTM! Import statement follows Angular style guide.
The import statement is well-structured and follows the correct path convention.
10-10: Verify lazy loading setup and check for circular dependencies.
While adding ArtemisSharedComponentModule to imports is appropriate for sharing components, we should ensure:
- The module is properly configured for lazy loading
- There are no circular dependencies between modules
✅ Verification successful
Based on the results, I can make a determination about the lazy loading and circular dependency concerns. Let me generate the final response:
No issues found with module imports and dependencies
The verification shows that:
SubmissionResultStatusModuleis properly imported as a regular module dependency - there's no need for lazy loading since it's a shared component module- No circular dependencies were detected - the module is imported by various feature modules but there are no bi-directional dependencies
- The module usage pattern follows Angular's best practices for shared component modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential circular dependencies and lazy loading setup
# Check for circular imports
rg -A 5 "SubmissionResultStatusModule"
rg -A 5 "ArtemisSharedComponentModule.*SubmissionResultStatusModule"
# Check lazy loading setup in routing
rg "loadChildren.*SubmissionResultStatusModule"
Length of output: 45203
src/main/webapp/app/entities/programming/programming-submission.model.ts (2)
9-11: LGTM! Changes align well with PR objectives.
The new properties effectively support the PR's goal of improving build status updates and timing estimations. The implementation:
- Maintains type safety with proper TypeScript types
- Uses optional properties appropriately
- Integrates smoothly with the existing model
Line range hint 23-23: Verify if the TODO comment is still relevant.
The TODO comment suggests potential removal of this method. Given the new changes for build status tracking, this might be a good time to address it.
src/test/javascript/spec/helpers/mocks/service/mock-programming-submission.service.ts (1)
3-3: LGTM!
The dayjs import is correctly added and properly used for date handling in the new method.
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseBuildConfigRepository.java (1)
21-23: Verify the implementation of getValueElseThrow.
The method references getValueElseThrow which is not visible in the current file. Let's verify its implementation.
✅ Verification successful
The previous search shows extensive usage of getValueElseThrow across the codebase, but we need to find its actual implementation. Let's search in the repository base classes to find the implementation.
Let's search in the base repository package with a more generic pattern to find the implementation, and also check the interface definition.
Implementation of getValueElseThrow is correct and properly handles error cases
The method getValueElseThrow is implemented in the base repository class RepositoryImpl with two overloads:
getValueElseThrow(Optional<U> optional)- throwsEntityNotFoundExceptionwith entity namegetValueElseThrow(Optional<U> optional, ID id)- throwsEntityNotFoundExceptionwith entity name and ID
Both implementations properly handle the case when the entity is not found by throwing an appropriate exception with meaningful error messages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the implementation of getValueElseThrow method
# Expected: Find the method implementation in a parent class or utility class
# Search for the method implementation
ast-grep --pattern 'getValueElseThrow($$$)'
# Search for the class hierarchy
rg -l "class.*extends.*ArtemisJpaRepository"
Length of output: 37697
Script:
#!/bin/bash
# Search for the implementation of getValueElseThrow in repository base classes
rg -A 5 "protected.*getValueElseThrow" src/main/java/de/tum/cit/aet/artemis/core/repository/
Length of output: 94
Script:
#!/bin/bash
# Search for the implementation of getValueElseThrow in base classes with different modifiers
rg -A 5 "getValueElseThrow.*\{" src/main/java/de/tum/cit/aet/artemis/core/repository/base/
# Also search in the interface definition
rg -A 5 "getValueElseThrow" src/main/java/de/tum/cit/aet/artemis/core/repository/base/ArtemisRepository.java
Length of output: 1814
src/main/java/de/tum/cit/aet/artemis/exercise/dto/SubmissionDTO.java (1)
27-37: 🛠️ Refactor suggestion
Verify temporal consistency between dates.
The method should ensure that buildStartDate is before estimatedCompletionDate when both are provided. Consider adding validation to prevent invalid states.
Add validation before creating the DTO:
public static SubmissionDTO of(Submission submission, boolean isProcessing, ZonedDateTime buildStartDate, ZonedDateTime estimatedCompletionDate) {
+ if (buildStartDate != null && estimatedCompletionDate != null && buildStartDate.isAfter(estimatedCompletionDate)) {
+ throw new IllegalArgumentException("Build start date must be before estimated completion date");
+ }
if (submission instanceof ProgrammingSubmission programmingSubmission) {
// ... rest of the codeLet's verify the usage:
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java (2)
Line range hint 1-85: Well-structured DTO implementation
The overall implementation follows best practices:
- Uses Java records appropriately for DTOs
- Maintains single responsibility
- Provides clear documentation
- Follows minimal DTO pattern
39-39: Consider using actual timing information instead of hardcoded values
The current implementation passes hardcoded values (false for isProcessing and null for timing fields) to SubmissionDTO.of(). Since this DTO represents a finished build job, we should consider:
- Using actual timing information from the Result object for better tracking of build job states
- Verifying if
isProcessingshould always befalsefor finished jobs
Let's verify the impact of these null values on the UI:
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobQueueItem.java (3)
Line range hint 1-56: Implementation aligns well with PR objectives
The changes effectively support the build queue duration estimation system by adding necessary timing information while maintaining the class's immutability and DTO responsibilities. The implementation properly integrates with the broader goal of improving build status updates for users.
32-34:
Consider adding null validation for timing information
The constructor accesses nested timing properties without null checks. While records provide null safety for direct fields, nested access to jobTimingInfo properties could throw NPE.
Apply this defensive programming approach:
public BuildJobQueueItem(BuildJobQueueItem queueItem, ZonedDateTime buildCompletionDate, BuildStatus status) {
+ if (queueItem == null || queueItem.jobTimingInfo() == null) {
+ throw new IllegalArgumentException("Queue item and timing info must not be null");
+ }
this(queueItem.id(), queueItem.name(), queueItem.buildAgent(), queueItem.participationId(), Line range hint 1-19: Consider documenting version migration strategy
The class is used in shared distributed storage (Hazelcast/Redis) and modifications can break existing deployments. Consider adding a version field to support backward compatibility and documenting the migration strategy.
src/main/java/de/tum/cit/aet/artemis/programming/dto/ResultDTO.java (1)
66-66: Verify consistent usage of new SubmissionDTO.of parameters.
The change from using a single parameter to using four parameters might affect other parts of the codebase.
✅ Verification successful
Let me check the implementation of SubmissionDTO.of to understand the method signature and verify if all calls are consistent.
Let me try a different approach to find the SubmissionDTO class and its implementation.
Based on the code analysis, I can now provide the final response:
All calls to SubmissionDTO.of are consistent with the method signature
The SubmissionDTO.of method is defined with four parameters: (Submission submission, boolean isProcessing, ZonedDateTime buildStartDate, ZonedDateTime estimatedCompletionDate). All four occurrences found in the codebase use the correct parameter types and order:
ProgrammingExerciseParticipationResource.java: Uses all four parametersProgrammingMessagingService.java: Usessubmission, isProcessing, null, nullResultDTO.java: Usesresult.getSubmission(), false, null, nullFinishedBuildJobDTO.java: Usesresult.getSubmission(), false, null, null
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to SubmissionDTO.of to ensure consistent parameter usage
ast-grep --pattern 'SubmissionDTO.of($$$)'
Length of output: 965
Script:
#!/bin/bash
# Search for the SubmissionDTO.of method implementation
ast-grep --pattern 'of($$$) {
$$$
}'
Length of output: 164
Script:
#!/bin/bash
# Find and show the content of SubmissionDTO.java
fd "SubmissionDTO.java" --exec cat {}
Length of output: 6540
src/main/webapp/app/exercises/programming/participate/code-editor-student-container.component.html (1)
78-78: LGTM! Property binding aligns with PR objectives.
The addition of [showProgressBar]="true" to the <jhi-updating-result> component enhances the user experience by providing visual feedback during build status updates, which directly supports the PR's goal of improving build status visibility.
Let's verify the integration with build status updates:
✅ Verification successful
Property binding is properly implemented and integrated
The verification confirms that:
- The
showProgressBarinput property is properly declared inUpdatingResultComponentwith a default value offalse - The component correctly passes this property to its child
jhi-resultcomponent - The property binding in the code-editor-student-container aligns with the component's implementation
The addition of [showProgressBar]="true" will effectively enable the progress bar visualization in the build status updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the UpdatingResultComponent handles the showProgressBar property
# and is integrated with build status updates
# Check if UpdatingResultComponent properly declares and uses the showProgressBar input
ast-grep --pattern 'class UpdatingResultComponent {
$$$
@Input() showProgressBar: boolean;
$$$
}'
# Check if the component template uses this property for progress bar rendering
rg -g '*.html' 'showProgressBar' src/main/webapp/app/exercises/programming/shared/
Length of output: 312
Script:
#!/bin/bash
# Let's try a different approach to locate the UpdatingResultComponent and its usage
# Find the component file
fd "updating-result.component.ts"
# Search for showProgressBar in all TypeScript files
rg "showProgressBar" -t ts
# Search for the component usage in HTML files
rg "<jhi-updating-result" -t html
Length of output: 1780
Script:
#!/bin/bash
# Let's check the component's template and other instances of showProgressBar usage
# Check the updating-result component's template
cat src/main/webapp/app/exercises/shared/result/updating-result.component.html
# Check other instances where showProgressBar is used with jhi-updating-result
rg -B 2 -A 2 '<jhi-updating-result.*showProgressBar' -t html
Length of output: 692
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (2)
3-3: LGTM: Clean dependency injection implementation.
The new dependency is properly integrated following best practices:
- Specific imports without wildcards
- Final field with constructor injection
- Appropriate access level
Also applies to: 25-26, 42-43
121-122: LGTM: Clean integration of build processing notification.
The notification is well-placed within the entryAdded event handler and properly extracts required information from the BuildJobQueueItem.
src/main/webapp/app/exercises/shared/result/result.component.html (1)
7-15: Well-implemented queued status display!
Good practices observed:
- Follows new Angular syntax (@if instead of *ngIf)
- Clear user feedback with spinning icon and status message
- Test-friendly with proper ID attributes
src/main/webapp/i18n/en/editor.json (1)
34-36: LGTM! Well-structured status messages for build queue.
The new status messages effectively support the PR's goal of improving build status visibility. The strings are clear, properly formatted, and follow good UX practices by:
- Using "..." to indicate ongoing status
- Providing a concise ETA label
- Including helpful tooltip text that sets appropriate user expectations
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingSubmissionRepository.java (1)
162-170: Add documentation and consider method reuse.
The query looks efficient, but consider:
- Adding Javadoc to explain the method's purpose and how it differs from
findByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents. - Evaluating if this method could reuse the existing one by filtering the results, to avoid maintaining two similar queries.
Let's verify the indexes for optimal query performance:
✅ Verification successful
Indexes are properly configured for optimal query performance
The verification shows that:
- The query uses columns that are part of foreign key relationships (
participation_id) which automatically have indexes in the database. - The
commit_hashcolumn is indexed through thebuild_plan_idindex in the participation table. - The method is a simpler version of the existing
findByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents, optimized for cases where feedbacks and team students are not needed.
The original review suggestion about documenting the method's purpose is still valid, but the performance concern is addressed by the existing database schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if proper indexes exist for the query
# Look for index definitions in repository files and migration scripts
# Search for index definitions
rg -A 5 "CREATE.*INDEX" --type sql
# Search for @Index annotations
ast-grep --pattern '@Index($$$)'
Length of output: 344
Script:
#!/bin/bash
# Let's try a different approach to find index information and related entities
# Find the entity class for ProgrammingSubmission
fd -e java "ProgrammingSubmission.java"
# Look for any JPA annotations in the entity class
rg "@(Table|Index|Column)" -A 2 "ProgrammingSubmission.java"
# Check if there are any Liquibase/Flyway migration files
fd -e sql -e xml -e yaml changelog
# Look for the participation entity as well since it's joined
fd -e java "Participation.java"
rg "@(Table|Index|Column)" -A 2 "Participation.java"
Length of output: 3519
Script:
#!/bin/bash
# Let's check the entity classes with their full paths and the changelog content
# Check ProgrammingSubmission entity
rg "@(Table|Index|Column)" -A 2 "src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingSubmission.java"
# Check Participation entity
rg "@(Table|Index|Column)" -A 2 "src/main/java/de/tum/cit/aet/artemis/exercise/domain/participation/Participation.java"
# Check recent changelog files for any index creation on these tables
rg -l "programming_submission|participation" src/main/resources/config/liquibase/changelog/*.xml | while read -r file; do
echo "=== $file ==="
cat "$file" | grep -A 5 -B 5 "createIndex\|addColumn\|createTable"
done
# Look for similar existing queries to compare
rg "findByParticipationIdAndCommitHash" -A 2 -B 2
Length of output: 136112
src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java (2)
32-32: LGTM: Imports are appropriate and follow guidelines
The new imports are properly scoped and follow the coding guidelines by avoiding wildcard imports.
Also applies to: 35-35
208-209: Verify error handling in the service layer
The call to getBuildJobEstimatedQueueDuration should be wrapped in a try-catch block to handle potential service layer exceptions gracefully.
src/main/webapp/app/exercises/shared/result/updating-result.component.ts (2)
7-7: LGTM: Clean property declarations following Angular guidelines
The new imports and property declarations are well-structured and follow Angular style guidelines:
- Proper type definitions using TypeScript
- Consistent camelCase naming
- Clear separation of concerns with distinct properties for different aspects of build status
Also applies to: 38-38, 49-51
Line range hint 1-216: Verify integration with build agent availability system
The implementation aligns well with the PR objectives of improving build status updates. However, we should verify the integration with the build agent availability system mentioned in the PR objectives.
✅ Verification successful
Let me analyze the build agent availability integration by searching for relevant code.
Based on the code analysis, I can now provide a final verification response:
Integration with build agent availability system is properly implemented
The implementation correctly integrates with the build agent availability system through:
- Queue state tracking via
ProgrammingSubmissionState.IS_QUEUED - Build timing estimation through
fetchQueueReleaseDateEstimationByParticipationIdAPI - Build progress monitoring with
BuildTimingInfocontainingbuildStartDateandestimatedCompletionDate - Proper state transitions between queued and building states
- Local CI profile awareness for timing information display
The code follows good practices:
- Proper error handling for queue estimation failures
- Validation of timing information before display
- Cleanup of subscriptions in component destruction
- Clear separation of queue and build states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for build agent availability related code
rg -l "buildAgent|agentAvailability" --type ts
# Search for queue duration calculation logic
rg -l "queueDuration|estimateWaitTime" --type ts
# Look for related test files
fd -e spec.ts -e test.ts
Length of output: 90979
Script:
#!/bin/bash
# Search for build queue and timing estimation related code
rg -A 10 "getQueueReleaseDateEstimationByParticipationId|fetchQueueReleaseDateEstimation" --type ts
# Search for build timing info usage
rg -A 5 "BuildTimingInfo|buildStartDate|estimatedCompletionDate" --type ts
# Look for programming submission service implementation
rg -A 10 "class ProgrammingSubmissionService" --type ts
Length of output: 64470
src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java (1)
113-114: Verify initialization strategy for build metrics.
The current implementation resets metrics when copying build configurations. Please confirm if this is the intended behavior, or if these metrics should be preserved in some scenarios (e.g., when cloning exercises).
✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient context about how the ProgrammingExerciseBuildConfig is used across the codebase.
Resetting metrics in copy constructor is the correct approach
The current implementation that resets metrics (buildDurationSeconds and successfulBuildCount) to 0 when copying build configurations is the correct behavior. This is evident from the codebase usage patterns where:
- New configurations are created during exercise imports, clones, and test setups
- Build metrics are execution-specific statistics that shouldn't be carried over
- All observed usages create fresh build configurations or copy only the configuration parameters, not the metrics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any cases where ProgrammingExerciseBuildConfig is copied while preserving metrics
# Search for copy/clone operations of ProgrammingExerciseBuildConfig
rg -l "new ProgrammingExerciseBuildConfig\(" | xargs -I {} rg -A 10 "new ProgrammingExerciseBuildConfig\(" {}
Length of output: 13499
src/test/javascript/spec/component/exercises/shared/result.spec.ts (1)
Line range hint 1-25: LGTM! Test setup follows best practices
The test file follows Angular testing best practices with proper use of:
- Jest as the testing framework
- NgMocks for component mocking
- Specific imports instead of full module imports
- Proper test isolation
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (4)
9-10: LGTM: Import statements are well-organized
The new imports are appropriately organized and follow Java conventions. The constants and DTOs are properly imported for the new submission processing functionality.
Also applies to: 28-28, 36-36
55-65: LGTM: Constructor injection properly implemented
The ParticipationRepository is correctly injected through constructor injection, following the coding guidelines and dependency injection best practices.
Line range hint 86-114: LGTM: Well-structured method overloading
The changes effectively implement the new processing status feature while maintaining backward compatibility. The code properly handles both individual and team submissions with appropriate type checking.
170-172: LGTM: Clean utility method implementation
The topic construction method is well-implemented and follows the established pattern for other similar methods in the class.
src/main/webapp/app/exercises/shared/result/result.component.ts (4)
58-58: LGTM: New input properties for enhanced build status tracking
The new input properties are well-defined and properly typed, following Angular guidelines. They support the PR's objective of providing clearer build status updates.
Also applies to: 68-70
173-175: LGTM: Proper cleanup of interval timer
The cleanup of the interval timer in ngOnDestroy is correctly implemented, preventing potential memory leaks.
220-220: LGTM: Proper integration of queue status
The isQueued parameter is correctly integrated into the template status evaluation.
80-82: Verify memory management for interval timer
The new properties are well-defined for progress tracking. Ensure the interval timer is properly cleaned up across all component lifecycle scenarios.
✅ Verification successful
Let me gather more information to check if ngOnDestroy is properly implemented.
Let me search for the ngOnDestroy implementation since we can see the class implements OnDestroy.
Memory management for interval timer is properly implemented
The interval timer is correctly managed:
- Cleared in
ngOnDestroylifecycle hook - Cleared before setting new interval when
estimatedCompletionDatechanges - Self-cleared when estimated time reaches zero
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential memory leaks in interval cleanup
# Look for other places where estimatedDurationInterval might need cleanup
# Search for usage of estimatedDurationInterval
rg "estimatedDurationInterval" -A 5
# Search for potential lifecycle hooks where cleanup might be needed
ast-grep --pattern 'ngOnDestroy() {
$$$
}'
Length of output: 3045
Script:
#!/bin/bash
# Search for the entire ResultComponent class to check for ngOnDestroy implementation
ast-grep --pattern 'export class ResultComponent $$$'
# Also search for any file content containing ResultComponent
rg "class ResultComponent" -A 10
Length of output: 1364
Script:
#!/bin/bash
# Search for ngOnDestroy implementation in result.component.ts
rg "ngOnDestroy\(\)" -A 5 src/main/webapp/app/exercises/shared/result/result.component.ts
# Also get the context where intervals are cleared
rg "clearInterval" -B 5 src/main/webapp/app/exercises/shared/result/result.component.ts
Length of output: 1380
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
67-70: Add documentation for the new WebSocket topic constants
The constants follow the established WebSocket topic naming pattern and align with the PR's objective of improving build status updates. However, adding documentation would improve clarity.
Add Javadoc comments explaining the purpose of these constants:
+ /**
+ * WebSocket endpoint path for submission processing status updates.
+ */
public static final String SUBMISSION_PROCESSING = "/submissionProcessing";
+ /**
+ * WebSocket topic for real-time submission processing notifications.
+ * Used to notify clients about the status of their build jobs (queued/processing).
+ */
public static final String SUBMISSION_PROCESSING_TOPIC = "/topic" + SUBMISSION_PROCESSING;Let's verify the usage of these new constants:
✅ Verification successful
Constants are properly used and no hardcoded values found
The verification shows that:
- The constants are imported and used correctly in
ProgrammingMessagingService - No hardcoded values of these paths exist in the codebase
- The constants are used for both direct path construction and WebSocket messaging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new WebSocket topic constants
# Expected: Usage in WebSocket configuration and messaging services
# Search for direct usage of the constants
echo "Searching for direct usage of SUBMISSION_PROCESSING constants..."
rg "SUBMISSION_PROCESSING(_TOPIC)?" --type java
# Search for the literal paths to catch any hardcoded values
echo "Checking for hardcoded paths that should use the constants..."
rg '"/topic/submissionProcessing"|"/submissionProcessing"' --type java
Length of output: 1611
src/main/webapp/app/exercises/shared/result/result.utils.ts (2)
32-36: LGTM! Well-documented enum value addition.
The new IS_QUEUED status aligns with the PR's objective of differentiating between queued and processing states, following the established documentation pattern.
221-223: Verify test coverage and component usage.
The new queued state introduces a significant change in result status evaluation. Let's verify the implementation:
✅ Verification successful
Based on the search results, I can now provide a final response:
The queued state is properly handled in the codebase
The verification shows that:
- The
IS_QUEUEDstate is properly defined in the result utils and submission service - The template handles the queued state with a dedicated case in result.component.html
- The state is properly propagated through the submission service and result components
- Test coverage exists for the queued state in multiple spec files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage and component usage of evaluateTemplateStatus
# Find test files covering this functionality
echo "Checking test coverage..."
rg -l "evaluateTemplateStatus" "src/**/*.spec.ts"
# Find components using this function
echo "Checking component usage..."
rg -l "evaluateTemplateStatus" "src/**/*.component.ts"
# Check if the new IS_QUEUED status is properly handled in templates
echo "Checking template handling..."
rg "IS_QUEUED" "src/**/*.html"
Length of output: 503
Script:
#!/bin/bash
# Let's try with fd to find the correct paths first
echo "Finding relevant test files..."
fd -e spec.ts
echo "Finding relevant component files..."
fd -e component.ts
echo "Finding relevant template files..."
fd -e html
echo "Checking evaluateTemplateStatus usage..."
rg "evaluateTemplateStatus" -A 3
echo "Checking IS_QUEUED handling..."
rg "IS_QUEUED"
Length of output: 259387
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
87-89: LGTM! JobTimingInfo constructor updates are consistent and well-structured.
The updated constructor calls properly initialize test data with logical temporal relationships, supporting the enhanced build status update feature.
Also applies to: 284-284
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (3)
98-100: LGTM: Well-structured instance variable declarations
The instance variables are properly declared as private fields and follow the class's existing naming conventions.
131-133: LGTM: Proper initialization in setup method
The Hazelcast queue and map initialization in @BeforeEach ensures clean test state for each test case.
517-544: Verify test resilience against timing-sensitive operations
The test involves timing-sensitive operations and assertions. Let's verify its stability across different execution environments.
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (3)
41-41: LGTM: Import statements are properly organized.
The new imports for SubmissionDTO and SharedQueueManagementService are correctly placed and follow the package structure.
Also applies to: 60-60
97-98: LGTM: Field declaration follows best practices.
The field is correctly declared as final and uses Optional to handle the nullable service, following Java best practices for dependency injection.
103-104: LGTM: Constructor injection properly implemented.
The constructor correctly implements dependency injection for the new service, maintaining immutability by assigning to the final field.
Also applies to: 118-118
src/test/javascript/spec/component/shared/updating-result.component.spec.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/shared/updating-result.component.spec.ts
Show resolved
Hide resolved
.../java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIServiceTest.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
Show resolved
Hide resolved
BBesrour
changed the title
Integrated code lifecyle: Improve build status updates for usersIntegrated code lifecycle: Improve build status updates for users
…-build-notification
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (2)
312-345: Consider performance optimizations for queue duration estimationWhile the implementation is correct, there are opportunities for performance improvements:
- Cache the sorted queue to avoid repeated sorting
- Use early returns for better readability
public ZonedDateTime getBuildJobEstimatedQueueDuration(long participationId) { - if (queue.isEmpty() || this.buildAgentsCapacity > this.runningBuildJobCount + queue.size()) { + if (queue.isEmpty() || buildAgentsCapacity.get() > runningBuildJobCount.get() + queue.size()) { return ZonedDateTime.now(); } String buildJobId = getIdOfQueuedJobFromParticipation(participationId); if (buildJobId == null) { return ZonedDateTime.now(); } + // Cache the sorted queue to avoid repeated sorting in streams + var sortedQueue = queue.stream() + .sorted(new LocalCIPriorityQueueComparator()) + .toList(); + List<BuildJobQueueItem> jobsQueuedBefore = queue.stream() .sorted(new LocalCIPriorityQueueComparator()) .takeWhile(job -> !job.id().equals(buildJobId)) .toList();
401-404: Improve method name clarityThe method name
getBuildAgentsCapacityis misleading as it updates both capacity and running job count.- private void getBuildAgentsCapacity() { + private void updateBuildAgentMetrics() { buildAgentsCapacity = buildAgentInformation.values().stream() .mapToInt(BuildAgentInformation::maxNumberOfConcurrentBuildJobs) .sum();src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (2)
221-224: Improve error message for better debugging.The error message when catching
IllegalArgumentExceptioncould be more descriptive.Consider applying this change:
try { submissionOpt = submissionService.getLatestPendingSubmission(participationId, lastGraded); } catch (IllegalArgumentException ex) { - throw new EntityNotFoundException("participation", participationId); + throw new EntityNotFoundException( + String.format("Failed to find participation %d or its submissions: %s", + participationId, ex.getMessage())); }
247-248: Consider adding null check before setting participation to null.While unlikely, it's good practice to add a null check before operating on objects.
Consider applying this change:
// Remove participation, is not needed in the response. - programmingSubmission.setParticipation(null); + if (programmingSubmission != null) { + programmingSubmission.setParticipation(null); + }src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts (2)
Line range hint
73-104: Consider implementing cache size limits and cleanup.While the cache implementation is good, consider adding:
- A maximum size limit for
startedProcessingCache- An automatic cleanup mechanism for old entries
- A method to clear the cache on service destruction
This would prevent potential memory leaks in long-running sessions.
private startedProcessingCache: Map<string, BuildTimingInfo> = new Map<string, BuildTimingInfo>(); +private readonly MAX_CACHE_SIZE = 1000; // Adjust based on expected usage + +private cleanCache(): void { + if (this.startedProcessingCache.size > this.MAX_CACHE_SIZE) { + // Remove oldest entries + const entriesToRemove = Array.from(this.startedProcessingCache.keys()) + .slice(0, this.startedProcessingCache.size - this.MAX_CACHE_SIZE); + entriesToRemove.forEach(key => this.startedProcessingCache.delete(key)); + } +}
896-908: Add JSDoc comments for new public methods.Consider adding comprehensive JSDoc comments for the new public methods:
setLocalCIProfilegetIsLocalCIProfileThis would improve code maintainability and help other developers understand the purpose and usage of these methods.
+/** + * Sets the local CI profile flag to determine which build system is used. + * This affects how submissions are processed and how build timing is handled. + * @param isLocalCIProfile - Boolean indicating whether local CI profile is active + */ public setLocalCIProfile(isLocalCIProfile: boolean) { this.isLocalCIProfile = isLocalCIProfile; } +/** + * Returns the current state of the local CI profile flag. + * @returns Boolean indicating whether local CI profile is active + */ public getIsLocalCIProfile() { return this.isLocalCIProfile; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java(2 hunks)src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java
- src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java
🧰 Additional context used
📓 Path-based instructions (4)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts (1)
🔇 Additional comments (9)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (3)
101-102: LGTM!
The initialization properly sets up the build agent listener and initial capacity calculation.
379-399: LGTM!
The BuildAgentListener implementation correctly handles all agent lifecycle events and maintains capacity information.
406-423: LGTM!
The submission processing check and BuildTimingInfo record are well-implemented and properly documented.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (2)
276-277: Refer to previous review comment about error handling.
The current implementation of error handling for rejected build jobs has already been addressed in a previous review comment.
397-398: LGTM: Comprehensive timing information preserved.
The implementation correctly maintains all timing information, including submission date, build start date, completion date, and estimates. This aligns well with the PR's objective of improving build status updates.
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
97-98: LGTM! Constructor dependency injection follows best practices.
The addition of SharedQueueManagementService as an optional dependency is properly implemented using constructor injection.
Also applies to: 103-104, 118-118
src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts (3)
Line range hint 16-42: LGTM: Well-structured type definitions for build timing information.
The new types and enum values are properly defined to support the queue management functionality. The optional properties in BuildTimingInfo appropriately handle cases where timing information might not be available.
215-233: LGTM: Robust queue timing implementation.
The implementation properly handles:
- Timer management with cleanup
- Accurate time calculations
- Edge cases for queue estimation
Also applies to: 494-496
Line range hint 856-894: LGTM: Comprehensive cleanup implementation.
The unsubscribe logic properly handles:
- All subscription types
- WebSocket topics
- Cache cleanup
...in/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
Show resolved
Hide resolved
...in/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
Show resolved
Hide resolved
...in/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java
Show resolved
Hide resolved
src/main/webapp/app/exercises/programming/participate/programming-submission.service.ts
Show resolved
Hide resolved
Contains non-breaking migration
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
We want to provide users with better status about their build jobs. This is done by differentiating (in the UI) when a build job is queued, and when it is being processed. And also providing an estimation for each stage
Description
Build queue duration estimation
The queue duration estimation calculates the waiting time for a build job in the queue before it starts processing. It considers the availability of build agents and the jobs already running or queued. The system tracks agent availability by calculating the remaining processing times of currently running jobs and uses this information to simulate job assignments. For the queued jobs, a simulation determines when each job will be assigned to the next available agent, updating agent availability based on job durations. If the number of queued jobs before the target job is fewer than the available agents, the target job can start immediately once an agent is free. Otherwise, the algorithm calculates the time at which the next agent becomes available to start the target job.
Server Client communication
Previously, the server sends the client a message via websockets when a new submission is available, this triggers the client to show
building and testingUI. Now, it changes it to Queued. When the build starts processing, the server now sends another message with the build timing information (estimated completion date, and build start date), this will trigger the progress bar animation.Steps for Testing
On Non-LocalCI servers (TS5, TS6)
On LocalCI servers (TS1-4)
Prerequisites:
1 instructor
Server with ICL
Create a programming exercise (enable online and offline IDE)
After creation, make sure that the status showing the template/solution build plans doesnt show a progress bar.
As a student, submit from the code editor and make sure the that result component is intuitive and provides "accurate" information
As a student, submit from IDE (clone and push) and make sure that the result component (whether in the code editor or the course overview) is intuitive and provides "accurate" information.
Exam Mode Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests