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 the handling of build job timing and processing across various components in the system. Key updates include modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
Outdated
Show resolved
Hide resolved
.../java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java
Outdated
Show resolved
Hide resolved
...in/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
Outdated
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
Outdated
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
Outdated
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
Outdated
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
|
BBesrour
dismissed stale reviews from coolchock, EneaGore, SimonEntholzer, and coderabbitai[bot]
via
dd80d79
…-build-notification
BBesrour
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
BBesrour
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
| @@ -105,13 +109,18 @@ public class LocalCITriggerService implements ContinuousIntegrationTriggerServic | |||
|
|
|||
| private final ProgrammingExerciseBuildConfigService programmingExerciseBuildConfigService; | |||
|
|
|||
| private static final int DEFAULT_BUILD_DURATION = 17; | |||
|
|
|||
| private static final double BUILD_DURATION_SAFETY_FACTOR = 1.2; | |||
There was a problem hiding this comment.
What is the rationale behind 1.2?
I think it is always good to document something like this (does not need to be changed in this PR)
| isBuilding = input<boolean>(); | ||
| isQueued = input<boolean>(); |
There was a problem hiding this comment.
Are the inputs required? If yes make it explicit (does not need to be changed, just a general comment)
| isBuilding = input<boolean>(); | |
| isQueued = input<boolean>(); | |
| isBuilding = input.required<boolean>(); | |
| isQueued = input.required<boolean>(); |
| @Input() showProgressBar = false; | ||
| @Input() showProgressBarBorder = false; |
There was a problem hiding this comment.
I think in general we should only add input signals for new code, but does not need to be changed in this PR
DEPLOY ONLY ON TS3!!
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)1. Make sure that pushing code (and the result component that show the status) behavior didnt change.On LocalCI servers (TS1-4)
Prerequisites:
/queue-duration-estimation). Make sure that it completed in less than 100msExam 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
SubmissionProcessingDTOclass for managing submission processing details.ProgrammingExerciseBuildStatisticsclass for tracking build statistics.Bug Fixes
Documentation
Style
Tests