Development: Improve View Build Logs code
#10180
Conversation
github-actions
bot
added
tests
server
WalkthroughThe pull request introduces significant changes to how build logs are handled in the Artemis application. The modifications involve removing the method for retrieving build log entries, changing the data type for build logs from an array to a single string, and updating the corresponding service, component, and test files. The changes simplify the log retrieval and display process, moving from a detailed, structured log representation to a more straightforward string-based approach. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
✨ Finishing Touches
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
🧹 Nitpick comments (2)
src/main/webapp/app/localci/build-queue/build-queue.component.html (1)
894-897: LGTM! The implementation follows the new Angular syntax guidelines.The code correctly uses
@ifand@elsedirectives instead of the older*ngIfsyntax.Consider these improvements for better accessibility and readability:
@if (rawBuildLogsString) { - <pre>{{ rawBuildLogsString }}</pre> + <pre class="build-logs" role="log" aria-label="Build logs" tabindex="0">{{ rawBuildLogsString }}</pre> } @else { - <span jhiTranslate="artemisApp.buildQueue.logs.noLogs"></span> + <p class="text-muted" jhiTranslate="artemisApp.buildQueue.logs.noLogs"></p> }Add these styles to your component's CSS:
.build-logs { background-color: #f8f9fa; padding: 1rem; border-radius: 0.25rem; white-space: pre-wrap; word-wrap: break-word; max-height: 500px; overflow-y: auto; }src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (1)
377-377: Consider using more specific expectation.Using
toHaveBeenCalled()instead oftoHaveBeenCalledOnce()makes the test less precise. The original expectation would catch potential duplicate calls that could indicate inefficient behavior.- expect(mockBuildQueueService.getBuildJobStatistics).toHaveBeenCalled(); + expect(mockBuildQueueService.getBuildJobStatistics).toHaveBeenCalledOnce();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildLogResource.java(0 hunks)src/main/webapp/app/entities/programming/build-log.model.ts(1 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.html(1 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.ts(2 hunks)src/main/webapp/app/localci/build-queue/build-queue.service.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(0 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts(2 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts(19 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildLogResource.java
- src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts
🧰 Additional context used
📓 Path-based instructions (5)
src/main/webapp/app/entities/programming/build-log.model.ts (1)
src/main/webapp/app/localci/build-queue/build-queue.service.ts (1)
src/main/webapp/app/localci/build-queue/build-queue.component.ts (1)
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}}
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (8)
src/main/webapp/app/localci/build-queue/build-queue.component.ts (4)
207-207: LGTM! Good practice initializing the string.The string initialization is explicit and follows TypeScript best practices.
431-432: LGTM! Proper state reset.Good practice clearing the logs and resetting the ID before loading new logs.
437-438: LGTM! Clean type handling.The type handling for the build logs response is correct and matches the service return type.
450-463: LGTM! Improved file download implementation.The implementation:
- Properly creates a Blob with correct MIME type
- Uses URL.createObjectURL for efficient handling
- Correctly cleans up the URL object to prevent memory leaks
src/main/webapp/app/entities/programming/build-log.model.ts (1)
12-12: Verify the impact of optional time property.Making the
timeproperty optional could affect code that assumes its presence. Please ensure that all consumers ofBuildLogLineshandle the case wheretimeis undefined.Run the following script to find potential issues:
src/main/webapp/app/localci/build-queue/build-queue.service.ts (1)
Line range hint
197-202: LGTM! Clean and effective implementation.The changes align well with the PR objectives:
- Return type changed to
Observable<string>for simpler log handling- Response type correctly set to 'text'
- Error handling maintained with user-friendly message
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (2)
665-666: LGTM! Test data updated to match new implementation.The changes correctly reflect the transition from array-based to string-based log handling:
- Test data simplified to multi-line string
- Expectation updated to check
rawBuildLogsStringAlso applies to: 671-671
673-693: LGTM! Comprehensive download functionality tests.The new tests thoroughly cover the Blob and URL handling:
- Blob creation with correct type
- URL object lifecycle (create and revoke)
- Download trigger via anchor element
- Cleanup of resources
src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts
Show resolved
Hide resolved
ole-ve
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts
Outdated
Show resolved
Hide resolved
BBesrour
dismissed stale reviews from coderabbitai[bot] and coolchock
via
c71c643
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/webapp/app/localci/build-queue/build-queue.component.ts (1)
451-457: Consider adding error type information for better error handling.The error handling in the catch block could be more specific by typing the error parameter.
- } catch (error) { + } catch (error: unknown) {src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (2)
680-689: Consider improving test assertions for error handling.The test should verify both success and failure scenarios of the download functionality.
const downloadSpy = jest.spyOn(DownloadUtil, 'downloadFile'); component.downloadBuildLogs(); expect(downloadSpy).toHaveBeenCalledOnce(); expect(downloadSpy).toHaveBeenCalledWith(mockBlob, `${buildJobId}.log`); - expect(alertServiceErrorStub).toHaveBeenCalled(); + expect(alertServiceErrorStub).not.toHaveBeenCalled(); + + // Test error scenario + downloadSpy.mockImplementationOnce(() => { throw new Error('Download failed'); }); + component.downloadBuildLogs(); + expect(alertServiceErrorStub).toHaveBeenCalledWith('artemisApp.buildQueue.logs.downloadError');
690-696: Consider removing redundant test.The second download test appears redundant as it's testing the same functionality. Consider removing it or testing a different scenario.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/webapp/app/entities/programming/build-log.model.ts(0 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.ts(3 hunks)src/main/webapp/i18n/de/buildQueue.json(1 hunks)src/main/webapp/i18n/en/buildQueue.json(1 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts(5 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/entities/programming/build-log.model.ts
🧰 Additional context used
📓 Path-based instructions (3)
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/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/localci/build-queue/build-queue.component.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-style
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: Build and Push Docker Image
- GitHub Check: server-tests
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
🔇 Additional comments (7)
src/main/webapp/app/localci/build-queue/build-queue.component.ts (3)
31-31: LGTM!The imports are correctly added and properly organized.
Also applies to: 37-37
208-208: LGTM!The property is correctly typed and initialized.
432-444: LGTM!The
viewBuildLogsmethod correctly handles the build logs as a string and properly manages component state.src/main/webapp/i18n/en/buildQueue.json (1)
82-83: LGTM!The error messages are clear, consistent, and properly organized.
src/main/webapp/i18n/de/buildQueue.json (1)
82-83: LGTM!The German translations are correctly formatted and follow the informal style guide (using "du/dein" form).
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (2)
270-271: LGTM!The AlertService is properly injected and mocked for testing.
Also applies to: 285-285, 291-292
672-678: LGTM!The test correctly verifies the log content handling.
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
We removed an unnecessary endpoint. Instead, we display the text file as is and we create a blob from the received text to download the file
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
Performance Tests
Test Coverage
Summary by CodeRabbit
Summary by CodeRabbit
Breaking Changes
Bug Fixes
Refactor
Tests