Integrated code lifecycle: Save and update build job to database at the start of the process
#10001
Conversation
github-actions
bot
added
server
…uild-job-status # Conflicts: # src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java
github-actions
bot
added
tests
buildagent
github-actions
bot
added
the
database
…uild-job-status # Conflicts: # src/main/webapp/app/course/manage/course-management.module.ts
|
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. 🔧 pmd (7.8.0)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.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/service/SharedQueueProcessingService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. WalkthroughThis pull request introduces comprehensive enhancements to the build job management system in the Artemis application. The changes focus on expanding build job status tracking, introducing new statuses like Changes
Possibly related PRs
Suggested Reviewers
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 9
🔭 Outside diff range comments (1)
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (1)
Line range hint
215-245: Add missing buildSubmissionDate in mock dataThe second mock job (id: '6') is missing the
buildSubmissionDatefield while the first one has it. This inconsistency could lead to unreliable test results.Add the missing field:
{ id: '6', // ... other fields ... + buildSubmissionDate: dayjs('2023-01-06'), buildStartDate: dayjs('2023-01-06'), buildCompletionDate: dayjs('2023-01-06'), // ... remaining fields ... }
🧹 Nitpick comments (23)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobsStatisticsDTO.java (1)
35-35: Consider exposingotherBuildsor handling unexpected statusesWhile
otherBuildsis included intotalBuilds, it is not exposed separately in the DTO. To improve clarity, consider logging unexpected statuses or addingotherBuildsto the DTO.src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (2)
61-84: Refactor to eliminate duplicate subscription logic ingetBuildJobStatisticsThe method
getBuildJobStatisticscontains duplicate code for handling statistics retrieval. Consider refactoring to reduce duplication and improve maintainability.Here's a possible refactor:
getBuildJobStatistics(span: SpanType = SpanType.WEEK) { this.route.paramMap.pipe(take(1)).subscribe((params) => { const courseId = Number(params.get('courseId')); const statsObservable = courseId ? this.buildQueueService.getBuildJobStatisticsForCourse(courseId, span) : this.buildQueueService.getBuildJobStatistics(span); statsObservable.subscribe({ next: (res: BuildJobStatistics) => { this.updateDisplayedBuildJobStatistics(res); }, error: (res: HttpErrorResponse) => { onError(this.alertService, res); }, }); }); }
21-21: Review imported modules inimportsarrayConsider verifying the necessity of both
ArtemisSharedComponentModuleandArtemisSharedModulein theimportsarray to ensure optimal module usage.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIEventListenerService.java (2)
71-96: Consider optimizing logging within thecheckPendingBuildJobsStatusmethodTo prevent excessive logging when processing many pending build jobs, consider adjusting log levels or adding conditional logging to limit output.
44-64: Adhere to the 'least access' principle for inner classesThe inner listener classes
QueuedBuildJobItemListener,ProcessingBuildJobItemListener, andBuildAgentListenerare package-private. If they are not used outside this service, consider making themprivateto encapsulate them.src/main/webapp/app/localci/build-queue/build-queue.component.html (3)
Line range hint
461-472: Standardize translation keys for consistencyIn the headers, some translation keys use
artemisApp.buildQueue.buildJob.start, while others useartemisApp.buildQueue.buildJob.submissionDate. Ensure consistent terminology across translation keys.Consider using
artemisApp.buildQueue.buildJob.buildStartDatefor clarity and consistency:-<span jhiTranslate="artemisApp.buildQueue.buildJob.start"></span> +<span jhiTranslate="artemisApp.buildQueue.buildJob.buildStartDate"></span>
491-493: Add sorting icons to new table headersThe new columns for
submissionDatedo not include sorting functionality.Consider adding sorting capabilities to these columns:
<th jhiSortBy="buildSubmissionDate" class="finish-jobs-column"> <span jhiTranslate="artemisApp.buildQueue.buildJob.submissionDate"></span> <fa-icon [icon]="faSort" /> </th>Also applies to: 640-642
743-770: Ensure consistent date format in the date-time pickerThe date-time picker for
buildSubmissionDateFilterFromandbuildSubmissionDateFilterToshould have consistent labels and formats.Check that both date pickers display dates in the desired format and that the labels match the intended filter criteria.
src/main/java/de/tum/cit/aet/artemis/programming/domain/build/BuildStatus.java (1)
8-10: Update documentation to reflect allBuildStatusvaluesThe Javadoc comments describe the build statuses but are missing descriptions for the new statuses
TIMEOUTandMISSING.Consider updating the documentation to include all statuses:
/** * SUCCESSFUL: the build was successful * FAILED: the build failed * ERROR: the build produced an error * CANCELLED: the build was cancelled * QUEUED: the build is queued * BUILDING: the build is currently building * TIMEOUT: the build timed out * MISSING: the build is missing (e.g., not processed within expected time) */Also applies to: 14-14
src/main/webapp/app/entities/programming/build-job.model.ts (1)
38-38: Initialize new properties in constructorsThe newly added
buildSubmissionDateproperty inFinishedBuildJobis optional but may cause issues if not set.Ensure that the property is initialized appropriately in constructors or when instances are created.
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java (1)
22-23: Consider reordering temporal parameters for better logical flowThe temporal parameters in the record could be ordered more logically:
buildSubmissionDate,buildStartDate,buildCompletionDaterepresents the chronological sequence, but they're split across multiple lines. Consider grouping them together for better readability.public record FinishedBuildJobDTO(String id, String name, String buildAgentAddress, long participationId, long courseId, long exerciseId, BuildStatus status, - RepositoryType repositoryType, String repositoryName, RepositoryType triggeredByPushTo, ZonedDateTime buildSubmissionDate, ZonedDateTime buildStartDate, - ZonedDateTime buildCompletionDate, String commitHash, ResultDTO submissionResult) + RepositoryType repositoryType, String repositoryName, RepositoryType triggeredByPushTo, + ZonedDateTime buildSubmissionDate, ZonedDateTime buildStartDate, ZonedDateTime buildCompletionDate, + String commitHash, ResultDTO submissionResult)src/main/webapp/i18n/en/buildQueue.json (2)
37-41: Ensure consistent status terminologyThe build status terms should follow a consistent pattern. Currently, some use past tense ("Timed out") while others use present tense ("Building"). Consider standardizing all status terms.
"successful": "Successful", "building": "Building", "queued": "Queued", "missing": "Missing", -"timeout": "Timed out" +"timeout": "Timing out"
78-81: Group related statistics togetherThe new statistics entries for missing and timed out builds should be grouped with their related percentage metrics for better maintainability.
-"missingBuilds": "Missing Builds:", -"missingBuildsPercentage": "Missing Builds %:", -"timeoutBuilds": "Timed out Builds:", -"timeoutBuildsPercentage": "Timed out Builds %:", +"buildStats": { + "missing": { + "count": "Missing Builds:", + "percentage": "Missing Builds %:" + }, + "timeout": { + "count": "Timed out Builds:", + "percentage": "Timed out Builds %:" + } +}src/main/webapp/i18n/de/buildQueue.json (1)
78-81: Consider revising the translation for timeout buildsWhile the translations are grammatically correct, "Zeitüberschreitende Builds" is quite long. Consider a shorter alternative like "Timeout-Builds" which is more commonly used in German technical context.
- "timeoutBuilds": "Zeitüberschreitende Builds:", - "timeoutBuildsPercentage": "Zeitüberschreitende Builds %:", + "timeoutBuilds": "Timeout-Builds:", + "timeoutBuildsPercentage": "Timeout-Builds %:",src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html (2)
1-24: Add aria-label to the collapse button for better accessibilityThe collapse button should have an aria-label to improve accessibility.
- <button class="btn" (click)="isCollapsed = !isCollapsed" [attr.aria-expanded]="!isCollapsed" [attr.aria-controls]="'collapseQuestion'"> + <button class="btn" (click)="isCollapsed = !isCollapsed" [attr.aria-expanded]="!isCollapsed" [attr.aria-controls]="'collapseQuestion'" [attr.aria-label]="'Toggle statistics section'">
85-87: Add accessibility attributes to the pie chartThe ngx-charts-pie-chart should include accessibility attributes for screen readers.
- <ngx-charts-pie-chart [view]="[100, 100]" [results]="ngxData" [scheme]="ngxColor" [doughnut]="true" /> + <ngx-charts-pie-chart + [view]="[100, 100]" + [results]="ngxData" + [scheme]="ngxColor" + [doughnut]="true" + role="img" + aria-label="Build statistics pie chart" + />src/main/java/de/tum/cit/aet/artemis/programming/domain/build/BuildJob.java (1)
164-170: Consider adding validation in the setter method.While the getter/setter methods follow conventions, consider adding validation in setBuildSubmissionDate to ensure the submission date is not set to a future date.
public void setBuildSubmissionDate(ZonedDateTime buildSubmissionDate) { + if (buildSubmissionDate != null && buildSubmissionDate.isAfter(ZonedDateTime.now())) { + throw new IllegalArgumentException("Build submission date cannot be in the future"); + } this.buildSubmissionDate = buildSubmissionDate; }src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (2)
168-170: Consider using a constant for the timeout message.The timeout handling is correctly implemented, but the message comparison could be improved by using a constant to avoid potential typos and make maintenance easier.
Consider extracting the message to a constant:
+private static final String BUILD_JOB_TIMEOUT_MESSAGE = "Build job with id %s was timed out"; -else if (ex.getCause() instanceof TimeoutException && ex.getMessage().equals("Build job with id " + buildJob.id() + " was timed out")) { +else if (ex.getCause() instanceof TimeoutException && ex.getMessage().equals(String.format(BUILD_JOB_TIMEOUT_MESSAGE, buildJob.id()))) {
Line range hint
217-218: Consider adding error handling for the save operation.The pre-queue persistence is a good practice to prevent race conditions. However, it should handle potential database errors gracefully.
Consider adding error handling:
-buildJobRepository.save(new BuildJob(buildJobQueueItem, BuildStatus.QUEUED, null)); +try { + buildJobRepository.save(new BuildJob(buildJobQueueItem, BuildStatus.QUEUED, null)); +} catch (Exception e) { + log.error("Failed to save build job before queueing: {}", e.getMessage(), e); + throw new LocalCIException("Failed to initialize build job", e); +}src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
299-300: Consider extracting time constants and using test data builders.The date filtering test logic could be more maintainable with explicit constants and test data builders.
Consider these improvements:
+private static final int DATE_BUFFER_SECONDS = 10; -searchParams.add("startDate", jobTimingInfo.submissionDate().minusSeconds(10).toString()); -searchParams.add("endDate", jobTimingInfo.submissionDate().plusSeconds(10).toString()); +var submissionDate = jobTimingInfo.submissionDate(); +searchParams.add("startDate", submissionDate.minusSeconds(DATE_BUFFER_SECONDS).toString()); +searchParams.add("endDate", submissionDate.plusSeconds(DATE_BUFFER_SECONDS).toString());src/main/webapp/app/localci/build-queue/build-queue.component.ts (1)
532-549: Enhance date validation logicThe date validation logic could be simplified and made more robust.
Consider this refactor to improve readability and maintainability:
- if (!this.finishedBuildJobFilter.buildSubmissionDateFilterFrom?.isValid()) { - this.finishedBuildJobFilter.buildSubmissionDateFilterFrom = undefined; - this.localStorage.clear(FinishedBuildJobFilterStorageKey.buildSubmissionDateFilterFrom); - this.finishedBuildJobFilter.removeFilterFromFilterMap(FinishedBuildJobFilterStorageKey.buildSubmissionDateFilterFrom); - } else { - this.localStorage.store(FinishedBuildJobFilterStorageKey.buildSubmissionDateFilterFrom, this.finishedBuildJobFilter.buildSubmissionDateFilterFrom); - this.finishedBuildJobFilter.addFilterToFilterMap(FinishedBuildJobFilterStorageKey.buildSubmissionDateFilterFrom); - } + private handleDateFilter(date: dayjs.Dayjs | undefined, storageKey: string) { + if (!date?.isValid()) { + this.finishedBuildJobFilter[storageKey] = undefined; + this.localStorage.clear(storageKey); + this.finishedBuildJobFilter.removeFilterFromFilterMap(storageKey); + } else { + this.localStorage.store(storageKey, date); + this.finishedBuildJobFilter.addFilterToFilterMap(storageKey); + } + } + + filterDateChanged() { + this.handleDateFilter( + this.finishedBuildJobFilter.buildSubmissionDateFilterFrom, + FinishedBuildJobFilterStorageKey.buildSubmissionDateFilterFrom + ); + this.handleDateFilter( + this.finishedBuildJobFilter.buildSubmissionDateFilterTo, + FinishedBuildJobFilterStorageKey.buildSubmissionDateFilterTo + ); + + const { buildSubmissionDateFilterFrom, buildSubmissionDateFilterTo } = this.finishedBuildJobFilter; + this.finishedBuildJobFilter.areDatesValid = !buildSubmissionDateFilterFrom || !buildSubmissionDateFilterTo || + buildSubmissionDateFilterFrom.isBefore(buildSubmissionDateFilterTo); + }src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
440-455: LGTM: Enhanced error handling with clear status differentiationThe error handling has been improved with:
- Clear distinction between timeout and cancellation cases
- Appropriate status assignment (TIMEOUT, CANCELLED, FAILED)
- Proper logging with correct severity levels
However, consider extracting the error message constants to class-level constants for better maintainability.
+ private static final String CANCELLED_MSG_TEMPLATE = "Build job with id %s was cancelled."; + private static final String TIMEOUT_MSG_TEMPLATE = "Build job with id %s was timed out"; - String cancelledMsg = "Build job with id " + buildJob.id() + " was cancelled."; - String timeoutMsg = "Build job with id " + buildJob.id() + " was timed out"; + String cancelledMsg = String.format(CANCELLED_MSG_TEMPLATE, buildJob.id()); + String timeoutMsg = String.format(TIMEOUT_MSG_TEMPLATE, buildJob.id());src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (1)
604-604: Improve test descriptions for date validationWhile the date validation logic is correct, the test description "should validate correctly" is too generic. Consider making it more specific to describe what's being validated.
- it('should validate correctly', () => { + it('should validate date ranges and duration filters', () => {Also applies to: 612-612, 620-620
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241213200000_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (31)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobsStatisticsDTO.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/domain/build/BuildJob.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/domain/build/BuildStatus.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIEventListenerService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(7 hunks)src/main/webapp/app/admin/admin.module.ts(2 hunks)src/main/webapp/app/course/manage/course-management.module.ts(1 hunks)src/main/webapp/app/entities/programming/build-job.model.ts(2 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html(1 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.scss(1 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts(1 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.html(8 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.scss(0 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.ts(7 hunks)src/main/webapp/i18n/de/buildQueue.json(2 hunks)src/main/webapp/i18n/de/result.json(1 hunks)src/main/webapp/i18n/en/buildQueue.json(2 hunks)src/main/webapp/i18n/en/result.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTest.java(2 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(1 hunks)src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts(8 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/localci/build-queue/build-queue.component.scss
✅ Files skipped from review due to trivial changes (3)
- src/main/webapp/i18n/de/result.json
- src/main/webapp/i18n/en/result.json
- src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.scss
🧰 Additional context used
📓 Path-based instructions (26)
src/main/webapp/app/admin/admin.module.ts (1)
src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTest.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/main/java/de/tum/cit/aet/artemis/programming/domain/build/BuildStatus.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/build-job.model.ts (1)
src/main/webapp/app/course/manage/course-management.module.ts (1)
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/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (1)
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.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/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/test/javascript/spec/component/localci/build-queue/build-queue.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}}
src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.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/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobsStatisticsDTO.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/build/BuildJob.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/BuildJobManagementService.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/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/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/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/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/webapp/app/localci/build-queue/build-queue.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.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/i18n/de/buildQueue.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/main/webapp/app/localci/build-queue/build-queue.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/test/javascript/spec/component/localci/build-queue/build-queue.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/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIEventListenerService.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
📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTest.java (1)
Learnt from: Hialus
PR: ls1intum/Artemis#7451
File: src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.java:9-9
Timestamp: 2024-11-12T12:51:58.050Z
Learning: The use of wildcard imports is acceptable in this project as specified in the `.editorconfig` file.
🪛 Biome (1.9.4)
src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts
[error] 46-48: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (37)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobsStatisticsDTO.java (4)
8-8: Updated record declaration correctly includes new fields
The addition of timeOutBuilds and missingBuilds to the record declaration accurately reflects the new build statuses being tracked.
21-24: Initialization of new status counters is appropriate
Introducing variables for timeOutBuilds, missingBuilds, and otherBuilds ensures that all build statuses are accounted for.
26-32: Switch statement updated to handle new build statuses
Including TIMEOUT and MISSING cases in the switch statement ensures accurate categorization of build job results.
36-36: Return statement updated to include all necessary fields
The BuildJobsStatisticsDTO constructor is correctly invoked with all required parameters, ensuring the DTO is fully populated.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (1)
1-124: Overall implementation adheres to Angular style guidelines
The component is well-structured, follows the Angular style guide, and aligns with the project's coding standards.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (2)
38-38: Constructor simplified by removing unnecessary dependency
The removal of HazelcastInstance from the constructor reduces unnecessary coupling and simplifies the class.
48-75: Methods correctly update WebSocket clients with relevant information
The methods appropriately send updates to WebSocket clients, ensuring that the queued and processing job information is current.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIEventListenerService.java (1)
1-166: Service effectively enhances real-time event handling
The implementation of listeners and scheduled checks improves the robustness of build job status tracking and aligns with application requirements.
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (2)
217-220: Handle potential exceptions when scheduling future tasks
When using future.get(4, TimeUnit.SECONDS);, there is a possibility of TimeoutException, InterruptedException, or ExecutionException being thrown. Currently, these exceptions are not handled, which could lead to unexpected test failures.
Consider adding exception handling to manage these potential exceptions:
doAnswer(invocation -> {
try {
var future = scheduler.schedule(() -> inspectImageResponse, 3, TimeUnit.SECONDS);
return future.get(4, TimeUnit.SECONDS);
} catch (InterruptedException | ExecutionException | TimeoutException e) {
// Handle exceptions appropriately
throw new RuntimeException("Exception during future.get()", e);
}
}).when(inspectImageCmd).exec();268-270: Verify correct ordering of build jobs by submission date
The method findFirstByParticipationIdOrderByBuildSubmissionDateDesc is used to retrieve the latest build job. Ensure that buildSubmissionDate is the correct field for ordering, especially if multiple build jobs exist for a participation.
Run the following script to check for the usage and implementation of this method:
src/main/webapp/app/entities/programming/build-job.model.ts (1)
51-52: Maintain default values for new statistics properties
The new properties timeOutBuilds and missingBuilds in BuildJobStatistics are initialized to 0, which is good practice.
The initialization aligns with the existing pattern in the class.
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java (1)
68-68: LGTM: Factory method correctly maps all fields
The static factory method correctly maps all fields including the new buildSubmissionDate while maintaining the chronological order of date fields.
src/main/webapp/i18n/de/buildQueue.json (2)
37-41: LGTM: Build status translations are clear and consistent
The new build status translations are properly formatted and use informal language as required by the coding guidelines.
44-45: LGTM: Build submission date translation is accurate
The translation for "Build Abgabedatum" correctly represents the submission date concept.
src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTest.java (2)
24-24: LGTM: Import is properly organized
The import follows the existing pattern and is correctly placed in the service imports section.
83-85: LGTM: Service field follows established pattern
The new LocalCIEventListenerService field is properly autowired and follows the existing pattern of service declarations in the abstract test class.
src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.java (3)
82-82: LGTM! Consistent use of buildSubmissionDate.
The change maintains consistency with the previous modifications, ensuring all statistics are based on the submission time.
110-113: LGTM! Well-structured transactional method.
The method is properly annotated and follows JPA best practices for modifying queries.
124-133: Ensure comprehensive test coverage for the conditional update logic.
The method uses a CASE statement for conditional updates, which should be thoroughly tested to verify the behavior in both scenarios (when buildStartDate is NULL and when it's not).
Let's check for existing test coverage:
src/main/webapp/app/admin/admin.module.ts (1)
51-51: LGTM! Proper Angular module organization.
The BuildQueueComponent is correctly imported and added to the module's imports array, following Angular's module organization best practices.
Also applies to: 81-81
src/main/java/de/tum/cit/aet/artemis/programming/domain/build/BuildJob.java (2)
48-49: LGTM! Well-defined entity field.
The buildSubmissionDate field is properly annotated and follows naming conventions.
95-95: LGTM! Proper field initialization.
The buildSubmissionDate is correctly initialized from the queueItem's timing information.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
263-265: 🛠️ Refactor suggestion
Consider adding transaction management and error handling.
While the ID management is correct, the operation could benefit from explicit transaction management and better error handling.
Consider this improvement:
-buildJobRepository.findByBuildJobId(queueItem.id()).ifPresent(existingBuildJob -> {
- buildJob.setId(existingBuildJob.getId());
-});
+try {
+ return buildJobRepository.findByBuildJobId(queueItem.id())
+ .map(existingBuildJob -> {
+ buildJob.setId(existingBuildJob.getId());
+ return buildJobRepository.save(buildJob);
+ })
+ .orElseGet(() -> buildJobRepository.save(buildJob));
+} catch (Exception e) {
+ log.error("Could not save build job to database: {}", e.getMessage(), e);
+ return null;
+}Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java (1)
111-112: LGTM! Clean dependency injection and initialization.
The integration of BuildJobRepository follows best practices:
- Uses constructor injection
- Properly initializes the repository
- Maintains single responsibility principle
Also applies to: 119-119, 133-133
src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts (3)
31-32: LGTM: Filter options updated to use submission dates
The change from build start date to build submission date is consistent with the architectural changes, and the test data uses appropriate date values.
39-40: LGTM: Request parameters correctly updated
The HTTP request parameter validation is properly updated to match the new filter options, maintaining consistency in the test suite.
530-530: LGTM: Build statistics model expanded
The expected response now includes the new fields timeOutBuilds and missingBuilds, properly initialized to zero.
src/main/webapp/app/localci/build-queue/build-queue.component.ts (4)
19-25: LGTM: Proper module imports for standalone component
The imports follow Angular's best practices for standalone components and include all necessary modules.
96-99: LGTM: Build status enum expanded
New statuses MISSING, BUILDING, QUEUED, and TIMEOUT added to properly track all possible build states.
115-124: LGTM: Component migrated to standalone
The component has been properly configured as standalone with all necessary imports.
127-133: LGTM: Modern dependency injection pattern
Switched to using the inject() function for dependency injection, following Angular's modern patterns.
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (2)
267-268: LGTM! Proper standalone component configuration
The changes correctly reflect the migration to Angular's standalone component pattern.
550-551: LGTM! Comprehensive filter validation
The test cases thoroughly validate the filter functionality, including:
- Filter counting
- Local storage persistence
- Filter reset behavior
Also applies to: 570-571, 582-583, 596-597
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (2)
132-138: LGTM! Method provides safe access to processing job IDs
The implementation correctly avoids using streams with IMap for stability reasons and returns a defensive copy of the keys.
217-221: LGTM! Good separation of concerns
The new method encapsulates the status update logic for cancelled jobs, improving code organization and reusability.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java (1)
180-190: LGTM! Improved error handling and user feedback
The changes enhance error handling by:
- Distinguishing between timeout and general failures
- Providing more informative error messages
- Maintaining clean error propagation through CompletionException
src/main/webapp/app/course/manage/course-management.module.ts (1)
127-127: Verify if BuildQueueComponent import is necessary
The BuildQueueComponent has been moved to ArtemisAdminModule and is now imported here. Please verify:
- That this component is actually used within this module's templates
- If not used, consider removing this import to avoid unnecessary bundle size
Let's check for actual usage of BuildQueueComponent in this module's templates:
✅ Verification successful
BuildQueueComponent import is correctly used
The component is properly used within this module as:
- It's registered as a route component in
course-management.route.ts - It's referenced in the tab bar navigation at
course-management-tab-bar.component.html - The component uses selector
jhi-build-queueand is accessible via the/course-management/:id/build-queueroute
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if BuildQueueComponent is used in any template files within the course management module
# Search for build-queue selector in template files
echo "Searching for build-queue component usage in templates..."
rg -t html "<(\s)*build-queue" src/main/webapp/app/course/manage/
Length of output: 192
Script:
#!/bin/bash
# Let's try a different approach to find the component usage:
# 1. First, let's find the selector used by BuildQueueComponent
echo "Finding the BuildQueueComponent selector..."
rg -l "BuildQueueComponent" src/main/webapp/app
# 2. Let's also check if this component is exported from ArtemisAdminModule
echo -e "\nChecking ArtemisAdminModule exports..."
rg "export class ArtemisAdminModule" -A 10 src/main/webapp/app
Length of output: 650
Script:
#!/bin/bash
# Let's check the course management route and component to see if and where BuildQueueComponent is used
echo "Checking course-management.route.ts..."
rg "BuildQueueComponent" src/main/webapp/app/course/manage/course-management.route.ts -A 5
echo -e "\nChecking build-queue.component.ts selector..."
rg "@Component" src/main/webapp/app/localci/build-queue/build-queue.component.ts -A 3
echo -e "\nChecking course management components for build-queue usage..."
rg -t html "build-queue" src/main/webapp/app/course/manage/
Length of output: 1885
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts
Show resolved
Hide resolved
...in/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java
Outdated
Show resolved
Hide resolved
...ec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts
Show resolved
Hide resolved
...ec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts
Show resolved
Hide resolved
...main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobsStatisticsDTO.java
Show resolved
Hide resolved
remove useless line
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (3)
184-191: Consider extracting magic numbers into named constants.The timeout values (5 and 15 seconds) should be extracted into named constants at the class level for better maintainability and clarity.
+ private static final int BUILD_STATE_TRANSITION_TIMEOUT_SECONDS = 5; + private static final int BUILD_COMPLETION_TIMEOUT_SECONDS = 15; - await().atMost(5, TimeUnit.SECONDS).until(() -> { + await().atMost(BUILD_STATE_TRANSITION_TIMEOUT_SECONDS, TimeUnit.SECONDS).until(() -> { Optional<BuildJob> buildJobOptionalTemp = buildJobRepository.findFirstByParticipationIdOrderByBuildStartDateDesc(studentParticipation.getId()); return buildJobOptionalTemp.isPresent() && buildJobOptionalTemp.get().getBuildStatus() == BuildStatus.BUILDING; }); - await().atMost(15, TimeUnit.SECONDS).until(() -> { + await().atMost(BUILD_COMPLETION_TIMEOUT_SECONDS, TimeUnit.SECONDS).until(() -> { Optional<BuildJob> buildJobOptionalTemp = buildJobRepository.findFirstByParticipationIdOrderByBuildStartDateDesc(studentParticipation.getId()); return buildJobOptionalTemp.isPresent() && buildJobOptionalTemp.get().getBuildStatus() == BuildStatus.SUCCESSFUL; });
216-219: Extract timeout and delay values into named constantsThe delay (3 seconds) and timeout (4 seconds) values should be constants for better maintainability.
+ private static final int SIMULATED_BUILD_DELAY_SECONDS = 3; + private static final int BUILD_TIMEOUT_WAIT_SECONDS = 4; doAnswer(invocation -> { - var future = scheduler.schedule(() -> inspectImageResponse, 3, TimeUnit.SECONDS); - return future.get(4, TimeUnit.SECONDS); + var future = scheduler.schedule(() -> inspectImageResponse, SIMULATED_BUILD_DELAY_SECONDS, TimeUnit.SECONDS); + return future.get(BUILD_TIMEOUT_WAIT_SECONDS, TimeUnit.SECONDS); }).when(inspectImageCmd).exec();
277-277: Extract time difference into a named constantThe 6-minute time difference should be a named constant for better maintainability and clarity.
+ private static final int MISSING_BUILD_JOB_THRESHOLD_MINUTES = 6; - buildJob.setBuildSubmissionDate(ZonedDateTime.now().minusMinutes(6)); + buildJob.setBuildSubmissionDate(ZonedDateTime.now().minusMinutes(MISSING_BUILD_JOB_THRESHOLD_MINUTES));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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
🔇 Additional comments (3)
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (3)
207-211: Ensure timeout restoration in finally block
The original timeout value should be restored in a finally block to ensure it's always reset, even if the test fails.
Also applies to: 251-252
215-216: Remove redundant stubbing of inspectImageCmd.exec()
The doReturn stubbing is immediately overridden by the doAnswer stubbing.
Line range hint 1-290: Overall changes look good!
The test coverage is comprehensive and properly validates the build job lifecycle, including new states (QUEUED, BUILDING, TIMEOUT, MISSING) and their transitions. The test methods are well-structured and follow testing best practices.
BBesrour
changed the title
Integrated code lifecycle: Save and update build job to database iterativelyIntegrated code lifecycle: Save and update build job to database at the start of the process
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/domain/build/BuildStatus.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.java
Show resolved
Hide resolved
...ain/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIEventListenerService.java
Outdated
Show resolved
Hide resolved
...ain/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIEventListenerService.java
Outdated
Show resolved
Hide resolved
…uild-job-status # Conflicts: # src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.java # src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java # src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java # src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java # src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.java (2)
44-45:⚠️ Potential issuePerformance impact needs attention - Missing index on build_submission_date
The change from
buildStartDatetobuildSubmissionDaterequires an index for optimal query performance. The Liquibase changelog shows the column was added but without an index, which could impact query performance with large datasets.
82-82:⚠️ Potential issuePerformance impact needs attention - Missing index affects statistics query
The statistics query also uses the non-indexed
buildSubmissionDatecolumn, which could impact performance with large datasets.
🧹 Nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (1)
228-233: Consider batch processing for better performanceWhile the implementation is correct, consider optimizing database operations by updating multiple build job statuses in a single transaction, especially when dealing with bulk cancellations.
private void updateCancelledQueuedBuildJobsStatus(List<BuildJobQueueItem> queuedJobs) { - for (BuildJobQueueItem queuedJob : queuedJobs) { - buildJobRepository.updateBuildJobStatus(queuedJob.id(), BuildStatus.CANCELLED); - } + if (!queuedJobs.isEmpty()) { + buildJobRepository.updateBuildJobStatuses( + queuedJobs.stream().map(BuildJobQueueItem::id).toList(), + BuildStatus.CANCELLED + ); + } }src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (1)
63-71: Consider visibility and enhance documentation.
- If this method is only used within this class, consider making it private to follow the principle of least privilege.
- Consider enhancing the documentation to specify the WebSocket destination/topic where this information is being sent.
/** * Sends build agent information over websocket. This method is called when a new build agent is added or removed. + * The information is sent to the following destinations: + * - /topic/build-agent-summary for the build agent summary + * - /topic/build-agent-details/{agentName} for specific agent details * * @param agentName the name of the build agent */src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIEventListenerService.java (1)
134-151: Enhance logging for better observabilityConsider adding more detailed logging to help with debugging and monitoring.
Apply this diff:
private class ProcessingBuildJobItemListener implements EntryAddedListener<Long, BuildJobQueueItem>, EntryRemovedListener<Long, BuildJobQueueItem> { @Override public void entryAdded(com.hazelcast.core.EntryEvent<Long, BuildJobQueueItem> event) { - log.debug("CIBuildJobQueueItem added to processing jobs: {}", event.getValue()); + log.debug("CIBuildJobQueueItem added to processing jobs - ID: {}, Course: {}, Exercise: {}, Status: {}", + event.getValue().id(), event.getValue().courseId(), event.getValue().exerciseId(), BuildStatus.BUILDING); localCIQueueWebsocketService.sendProcessingJobsOverWebsocket(event.getValue().courseId()); buildJobRepository.updateBuildJobStatusWithBuildStartDate(event.getValue().id(), BuildStatus.BUILDING, event.getValue().jobTimingInfo().buildStartDate()); notifyUserAboutBuildProcessing(event.getValue().exerciseId(), event.getValue().participationId(), event.getValue().buildConfig().assignmentCommitHash(), event.getValue().jobTimingInfo().submissionDate(), event.getValue().jobTimingInfo().buildStartDate(), event.getValue().jobTimingInfo().estimatedCompletionDate()); } @Override public void entryRemoved(com.hazelcast.core.EntryEvent<Long, BuildJobQueueItem> event) { - log.debug("CIBuildJobQueueItem removed from processing jobs: {}", event.getOldValue()); + log.debug("CIBuildJobQueueItem removed from processing jobs - ID: {}, Course: {}, Exercise: {}", + event.getOldValue().id(), event.getOldValue().courseId(), event.getOldValue().exerciseId()); localCIQueueWebsocketService.sendProcessingJobsOverWebsocket(event.getOldValue().courseId()); } }src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java (1)
233-234: Consider adding transaction managementWhile the order of operations is correct, consider adding transaction management to ensure atomicity.
Apply this diff:
+ @Transactional private void triggerBuild(ProgrammingExerciseParticipation participation, String commitHashToBuild, RepositoryType triggeredByPushTo, boolean triggerAll) throws LocalCIException { // ... existing code ... // Save the build job before adding it to the queue in case it got quickly processed. Update statement would then fail. buildJobRepository.save(new BuildJob(buildJobQueueItem, BuildStatus.QUEUED, null)); queue.add(buildJobQueueItem); // ... existing code ... }src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (2)
197-205: Consider extracting magic numbers into constants.The timeout values (5 and 15 seconds) in the await conditions should be extracted into named constants for better maintainability and clarity.
+private static final int BUILD_STATUS_TIMEOUT_SECONDS = 5; +private static final int BUILD_COMPLETION_TIMEOUT_SECONDS = 15; -await().atMost(5, TimeUnit.SECONDS).until(() -> { +await().atMost(BUILD_STATUS_TIMEOUT_SECONDS, TimeUnit.SECONDS).until(() -> { Optional<BuildJob> buildJobOptionalTemp = buildJobRepository.findFirstByParticipationIdOrderByBuildStartDateDesc(studentParticipation.getId()); return buildJobOptionalTemp.isPresent() && buildJobOptionalTemp.get().getBuildStatus() == BuildStatus.BUILDING; }); -await().atMost(15, TimeUnit.SECONDS).until(() -> { +await().atMost(BUILD_COMPLETION_TIMEOUT_SECONDS, TimeUnit.SECONDS).until(() -> { Optional<BuildJob> buildJobOptionalTemp = buildJobRepository.findFirstByParticipationIdOrderByBuildStartDateDesc(studentParticipation.getId()); return buildJobOptionalTemp.isPresent() && buildJobOptionalTemp.get().getBuildStatus() == BuildStatus.SUCCESSFUL; });
269-302: Add documentation for the test scenario.The test method would benefit from explicit documentation explaining the test scenario, including:
- The purpose of the 6-minute offset
- The expected behavior when a build job is marked as missing
- The cleanup process
Add a comprehensive Javadoc:
/** * Tests the detection and handling of missing build jobs. * A build job is considered missing when: * 1. It has been in QUEUED state for more than 5 minutes * 2. It is no longer present in the build queue * * The test simulates this by: * 1. Creating a build job * 2. Setting its submission date to 6 minutes ago * 3. Clearing the build queue * 4. Verifying the job is marked as MISSING */ @Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") void testMissingBuildJobCheck() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIEventListenerService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(7 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(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java
- src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java
🧰 Additional context used
📓 Path-based instructions (6)
src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.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/LocalCIEventListenerService.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/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/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/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
🔇 Additional comments (16)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (6)
45-45: LGTM!
The BuildStatus import is necessary for the new status update functionality.
143-150: LGTM!
The method follows established patterns in the class:
- Creates a defensive copy to prevent external modifications
- Avoids streams with IMap for stability
- Has a clear single responsibility
217-217: LGTM!
The addition ensures build job statuses are properly updated in the database when cancelled, aligning with the PR's objective to improve build job status tracking.
250-252: LGTM!
The changes ensure proper status updates for all cancelled jobs while maintaining correct operation order:
- Get queued jobs
- Clear queue
- Update status in database
288-288: LGTM!
The addition maintains consistency with other cancellation methods by ensuring course-specific cancelled jobs are properly updated in the database.
319-319: LGTM!
The addition maintains consistency with other cancellation methods by ensuring participation-specific cancelled jobs are properly updated in the database.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (3)
40-49: Consider restricting method visibility.
The method has been changed from private to package-private. If this method is only used within this class, consider making it private to follow the principle of least privilege.
51-62: Consider restricting method visibility.
The method has been changed from private to package-private. If this method is only used within this class, consider making it private to follow the principle of least privilege.
34-38: LGTM! Verify service usage after dependency removal.
The constructor properly follows constructor injection and has been simplified by removing the HazelcastInstance dependency.
Let's verify that all service usages have been updated:
✅ Verification successful
Constructor changes are safe and properly implemented
The verification shows that:
- The service is only instantiated via Spring's dependency injection (no direct instantiations found)
- The only reference is in
LocalCIEventListenerServicewhich uses constructor injection - The service is properly annotated with
@Serviceand@Profile("localci & scheduling")
The removal of HazelcastInstance from the constructor is safe as there are no direct instantiations that would need updating, and Spring's DI will handle the updated constructor signature automatically.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining HazelcastInstance references in constructor calls
ast-grep --pattern 'new LocalCIQueueWebsocketService($$$hazelcastInstance$$$)'
Length of output: 204
Script:
#!/bin/bash
# Search for any references to LocalCIQueueWebsocketService in the codebase
rg "LocalCIQueueWebsocketService" -B 2 -A 2
# Also search for any bean configurations that might wire this service
fd "application.*\.yml|.*Config.*\.java" --exec rg -l "LocalCIQueueWebsocketService" {}
Length of output: 3362
src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.java (3)
17-17: LGTM!
The imports are correctly added and are required for the new update methods.
Also applies to: 21-21
125-157: LGTM! Well-implemented update methods
The new methods for updating build job status are well-implemented with:
- Proper transaction management using
@Transactional - Clear documentation explaining the purpose and behavior
- Efficient update queries using
@Modifying
44-45: Verify date comparison logic
The comparison between buildSubmissionDate and startDate parameter needs verification. Intuitively, a running job should be included when it started after the startDate, not when it was submitted.
Run the following script to analyze the usage:
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIEventListenerService.java (1)
75-80: 🛠️ Refactor suggestion
Improve code quality and performance
Two improvements needed:
- The magic number
5should be a configurable constant. ZonedDateTime.now()should be moved outside the loop as it's an expensive operation.
Apply this diff:
+ private static final String CHECK_JOB_STATUS_TIMEOUT_MINUTES = "${artemis.continuous-integration.check-job-status-timeout-minutes:5}";
+
@Scheduled(fixedRateString = "${artemis.continuous-integration.check-job-status-interval-seconds:300}", initialDelayString = "${artemis.continuous-integration.check-job-status-delay-seconds:60}", timeUnit = TimeUnit.SECONDS)
public void checkPendingBuildJobsStatus() {
log.info("Checking pending build jobs status");
List<BuildJob> pendingBuildJobs = buildJobRepository.findAllByBuildStatusIn(List.of(BuildStatus.QUEUED, BuildStatus.BUILDING));
+ ZonedDateTime now = ZonedDateTime.now();
for (BuildJob buildJob : pendingBuildJobs) {
- if (buildJob.getBuildSubmissionDate().isAfter(ZonedDateTime.now().minusMinutes(5))) {
+ if (buildJob.getBuildSubmissionDate().isAfter(now.minusMinutes(Integer.parseInt(CHECK_JOB_STATUS_TIMEOUT_MINUTES)))) {Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java (2)
45-46: LGTM!
The imports are correctly added and are required for the new build job functionality.
Also applies to: 48-48
115-116: LGTM!
The changes follow proper dependency injection patterns using constructor injection.
Also applies to: 128-128, 144-144
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (1)
219-233: Potential race condition in timeout simulation.
The current implementation might lead to flaky tests. The delay of 3 seconds with a timeout of 1 second might not consistently trigger the timeout condition due to system load or timing variations.
Consider increasing the margin between the timeout and delay:
-buildConfig.setTimeoutSeconds(1);
+buildConfig.setTimeoutSeconds(2);
-var future = scheduler.schedule(() -> inspectImageResponse, 3, TimeUnit.SECONDS);
+var future = scheduler.schedule(() -> inspectImageResponse, 10, TimeUnit.SECONDS);
-return future.get(4, TimeUnit.SECONDS);
+return future.get(12, TimeUnit.SECONDS);
...ain/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIEventListenerService.java
Show resolved
Hide resolved
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
Contains migration. Please don't deploy until code reviews
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Description
In the current implementation, we only save the build job to the database after it finishes. This PR changes that so that it is saved and iteratively updated. Thus, we added QUEUED, and BUILDING states.
We also added MISSING state. We periodically check whether queued and building jobs are actually queued/building or finished. If not, we mark them as missing. We also added a TIMEOUT state to distinguish between a "normal" failure and a timeout failure.
This PR also contains component migration.
Steps for Testing
Prerequisites:
Exam Mode Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
QUEUED,BUILDING,MISSING, andTIMEOUT.BuildQueueComponentfor enhanced build queue management.BuildJobStatisticsComponentto display detailed build job statistics.Enhancements
Bug Fixes
Documentation