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/SharedQueueProcessingService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. 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 selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (1)Pattern ⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (6)
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
…uild-job-status # Conflicts: # src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCITriggerService.java # src/main/webapp/app/localci/build-queue/build-queue.component.ts # src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts
BBesrour
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
BBesrour
added
deploy:artemis-test3
and removed
deployment-error
BBesrour
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
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