Integrated code lifecycle: View build logs in Artemis UI
#9990
Conversation
github-actions
bot
added
tests
server
|
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.7.0)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.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. WalkthroughThe pull request introduces enhancements to the build log management system by adding a method to parse build log entries in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (6)
src/main/webapp/app/localci/build-queue/build-queue.component.scss (1)
38-54: Consider modern layout alternatives to floatWhile the styling follows BEM naming conventions nicely, consider these improvements:
- The use of
floatandclearfor layout could be replaced with modern flexbox:- Ensure parent elements have explicit height for
height: inheritto work as expected.Consider this more maintainable alternative using flexbox:
.build-output { height: inherit; &__entry { + display: flex; + align-items: flex-start; &-date { width: 200px; margin-right: 10px; color: var(--secondary); font-weight: normal; - float: left; - clear: left; } &-text { margin-bottom: 0; color: var(--body-color); + flex: 1; } } }src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildLogResource.java (2)
59-72: Add debug logging for consistency with existing endpointThe new endpoint should follow the same logging pattern as the existing
getBuildLogForBuildJobmethod for consistency and debugging purposes.Add debug logging:
@GetMapping("build-log/{buildJobId}/entries") @EnforceAtLeastEditor public ResponseEntity<List<BuildLogEntry>> getBuildLogEntriesForBuildJob(@PathVariable String buildJobId) { + log.debug("REST request to get build log entries for build job {}", buildJobId); FileSystemResource buildLog = buildLogEntryService.retrieveBuildLogsFromFileForBuildJob(buildJobId); if (buildLog == null) { return ResponseEntity.notFound().build(); }
61-72: Consider adding response caching for performanceSince build logs are immutable once generated, consider adding caching to improve performance for frequently accessed logs.
Add cache control headers:
public ResponseEntity<List<BuildLogEntry>> getBuildLogEntriesForBuildJob(@PathVariable String buildJobId) { FileSystemResource buildLog = buildLogEntryService.retrieveBuildLogsFromFileForBuildJob(buildJobId); if (buildLog == null) { return ResponseEntity.notFound().build(); } var buildLogEntries = buildLogEntryService.parseBuildLogEntries(buildLog); if (buildLogEntries == null || buildLogEntries.isEmpty()) { return ResponseEntity.notFound().build(); } - return ResponseEntity.ok(buildLogEntries); + return ResponseEntity.ok() + .cacheControl(CacheControl.maxAge(1, TimeUnit.HOURS)) + .body(buildLogEntries); }src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
348-370: Test structure is good, but timestamp comparison could be more precise.The test effectively validates the build log retrieval endpoint with proper setup and cleanup. However, the timestamp comparison could be more precise.
Consider this improvement for more precise timestamp validation:
-assertThat(buildLogEntry.getTime().toString()).contains(time); +assertThat(ZonedDateTime.parse(time)).isEqualTo(buildLogEntry.getTime());This ensures exact timestamp matching rather than just string containment.
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (1)
682-696: Consider splitting the test case for better isolationWhile the test covers both viewing and downloading logs, it would be better to split it into two separate test cases for better isolation and clarity:
- Test for viewing build logs
- Test for downloading build logs
This would make the tests more focused and easier to maintain.
- it('should download build logs', () => { + it('should view build logs', () => { const buildJobId = '1'; - jest.spyOn(window, 'open').mockImplementation(); mockBuildQueueService.getBuildJobLogs = jest.fn().mockReturnValue(of(buildLogEntries)); component.viewBuildLogs(undefined, buildJobId); expect(mockBuildQueueService.getBuildJobLogs).toHaveBeenCalledWith(buildJobId); expect(component.rawBuildLogs).toEqual(buildLogEntries); + }); + it('should download build logs', () => { + const buildJobId = '1'; + jest.spyOn(window, 'open').mockImplementation(); + component.displayedBuildJobId = buildJobId; + component.downloadBuildLogs(); expect(window.open).toHaveBeenCalledWith(`/api/build-log/${component.displayedBuildJobId}`, '_blank'); });src/main/webapp/app/localci/build-queue/build-queue.component.html (1)
883-911: Consider enhancing the build logs modal with accessibility and user feedback.The build logs modal implementation looks good but could benefit from some improvements:
- Add accessibility attributes to the logs table
- Add loading state indicator while fetching logs
- Add error handling UI for failed log fetches
Consider applying these changes:
<div class="modal-body"> + @if (isLoading) { + <div class="text-center"> + <div class="spinner-border" role="status"> + <span class="visually-hidden">Loading...</span> + </div> + </div> + } @else if (error) { + <div class="alert alert-danger" role="alert"> + {{ error }} + </div> + } @else { - <table class="table table-borderless"> + <table class="table table-borderless" role="table" aria-label="Build Logs"> <tbody> @for (logEntry of rawBuildLogs; track logEntry) { - <tr class="build-output__entry"> + <tr class="build-output__entry" role="row"> - <td class="build-output__entry-date">{{ logEntry.time | artemisDate: 'short' : true }}</td> - <td class="build-output__entry-text">{{ logEntry.log }}</td> + <td class="build-output__entry-date" role="cell">{{ logEntry.time | artemisDate: 'short' : true }}</td> + <td class="build-output__entry-text" role="cell">{{ logEntry.log }}</td> </tr> } </tbody> </table> + } </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildLogResource.java(3 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.html(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.scss(1 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.service.ts(2 hunks)src/main/webapp/i18n/de/buildQueue.json(1 hunks)src/main/webapp/i18n/en/buildQueue.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(2 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts(4 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts(3 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts
- src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java
- src/main/webapp/app/localci/build-queue/build-queue.component.ts
🧰 Additional context used
📓 Path-based instructions (9)
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/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/java/de/tum/cit/aet/artemis/programming/web/localci/BuildLogResource.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.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/BuildLogEntryService.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.service.ts (1)
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/main/webapp/app/localci/build-queue/build-queue.component.ts (1)
🔇 Additional comments (7)
src/main/webapp/i18n/en/buildQueue.json (1)
78-80: LGTM! Translation keys follow existing patterns
The new translation keys are well-structured and consistent with the existing naming patterns.
src/main/webapp/i18n/de/buildQueue.json (1)
78-80: LGTM: German translations follow guidelines.
The translations use the informal "du" form as required and are consistent with their English counterparts.
src/main/webapp/app/localci/build-queue/build-queue.service.ts (1)
193-203: LGTM: Implementation follows service patterns.
The new getBuildJobLogs method:
- Follows the established error handling pattern
- Uses consistent URL construction
- Returns appropriate type Observable<BuildLogEntry[]>
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (2)
22-22: LGTM: Well-structured mock data for build logs
The mock data setup for build logs is comprehensive and follows the established pattern in the test file.
Also applies to: 276-285
44-44: LGTM: Mock service properly extended
The BuildQueueService mock is correctly extended with the new getBuildJobLogs method.
src/main/webapp/app/localci/build-queue/build-queue.component.html (2)
507-507: LGTM! Modal reference renamed for better clarity.
The change from content to filterModal improves code readability by using a more descriptive name.
620-620: LGTM! Build logs viewing link properly implemented.
The implementation correctly uses:
- Translation key for internationalization
- Click handler to open the modal
- Consistent styling with other table actions
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/localci/build-queue/build-queue.component.ts (2)
163-164: Consider initializing properties in the constructor.These properties could be initialized in the constructor for better organization and to ensure they're properly set up when the component is created.
472-474: Add type safety for modal parameter.The
modalparameter is typed asany. Consider using a more specific type from@ng-bootstrap/ng-bootstrap.- openModal(modal: any, fullscreen?: boolean, size?: 'sm' | 'lg' | 'xl', scrollable = true, keyboard = true) { + openModal(modal: NgbModal | string, fullscreen?: boolean, size?: 'sm' | 'lg' | 'xl', scrollable = true, keyboard = true) { this.modalService.open(modal, { size, keyboard, scrollable, fullscreen }); }src/main/webapp/app/localci/build-queue/build-queue.component.html (1)
883-916: Consider accessibility improvements for the build logs modalWhile the basic functionality is implemented correctly, consider the following accessibility improvements:
- Add aria-label attributes for better screen reader support
- Consider adding keyboard shortcuts for modal navigation
- Add loading state indication while fetching logs
Here's a suggested implementation:
<ng-template #buildLogsModal let-modal> - <div class="modal-header"> + <div class="modal-header" role="dialog" aria-labelledby="buildLogsModalTitle"> <h5 class="modal-title"> - <span jhiTranslate="artemisApp.buildQueue.logs.title"></span> + <span id="buildLogsModalTitle" jhiTranslate="artemisApp.buildQueue.logs.title"></span> <span> {{ this.displayedBuildJobId }}</span> </h5> <button type="button" class="btn-close" aria-label="Close" (click)="modal.dismiss()"></button> </div> <div class="modal-body"> + @if (isLoadingLogs) { + <div class="text-center"> + <span class="spinner-border" role="status"></span> + <span class="sr-only">Loading logs...</span> + </div> + } <table class="table table-borderless"> <tbody> @for (logEntry of rawBuildLogs; track logEntry) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildLogResource.java(3 hunks)src/main/webapp/app/entities/programming/build-log.model.ts(1 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.html(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.ts(4 hunks)src/main/webapp/app/shared/pipes/artemis-date.pipe.ts(6 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(2 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java
- src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildLogResource.java
🧰 Additional context used
📓 Path-based instructions (5)
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/webapp/app/entities/programming/build-log.model.ts (1)
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/main/webapp/app/localci/build-queue/build-queue.component.ts (1)
src/main/webapp/app/shared/pipes/artemis-date.pipe.ts (1)
🔇 Additional comments (5)
src/main/webapp/app/shared/pipes/artemis-date.pipe.ts (1)
39-39: LGTM! Well-structured milliseconds support implementation.
The changes to add milliseconds support are well-implemented, following Angular's style guide and maintaining consistency with existing code patterns.
Also applies to: 51-53, 64-64, 94-94, 99-99, 150-150, 155-157
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (2)
22-22: LGTM: Mock data setup is well-structured
The mock data for build log entries follows the established patterns in the test file and provides good test coverage for the new functionality.
Also applies to: 276-285
44-44: LGTM: Service mock setup is consistent
The mock service method for getBuildJobLogs is properly added to the mockBuildQueueService object.
src/main/webapp/app/localci/build-queue/build-queue.component.html (2)
507-507: LGTM: Modal reference update
The filter modal template reference is correctly updated to match the new naming convention.
Also applies to: 745-745
620-620: LGTM: View logs link implementation
The view logs link is properly integrated with the new modal functionality.
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildLogResource.java (1)
59-78: Add debug logging for better observability.The method implementation looks good with proper security, error handling, and response types. Consider adding debug logging similar to the existing
getBuildLogForBuildJobmethod.@GetMapping("build-log/{buildJobId}/entries") @EnforceAtLeastEditor public ResponseEntity<List<BuildLogDTO>> getBuildLogEntriesForBuildJob(@PathVariable String buildJobId) { + log.debug("REST request to get the build log entries for build job {}", buildJobId); FileSystemResource buildLog = buildLogEntryService.retrieveBuildLogsFromFileForBuildJob(buildJobId); if (buildLog == null) { return ResponseEntity.notFound().build(); } var buildLogEntries = buildLogEntryService.parseBuildLogEntries(buildLog); if (buildLogEntries == null || buildLogEntries.isEmpty()) { return ResponseEntity.notFound().build(); } return ResponseEntity.ok(buildLogEntries); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildLogResource.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildLogResource.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/BuildLogEntryService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildLogResource.java (1)
5-6: LGTM!
The added imports are necessary for the new functionality.
Also applies to: 21-21
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java (1)
5-7: LGTM!
The added imports are necessary for the new file parsing functionality.
Also applies to: 13-13
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java
Show resolved
Hide resolved
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
In the build overview page, we want to be able to view the build logs in Artemis UI instead downloading them. Downlaoding should still be possible
Steps for 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
Bug Fixes
Documentation
Tests