Development: Improve client test coverage
#11016
Conversation
github-actions
bot
added
client
WalkthroughThis update adds new and reorganized unit tests for several Angular services and utility functions, focusing on error handling, control flow, and correct data processing. The changes include expanded test coverage for credential utilities, passkey management, chart fallback states, image EXIF utilities, canvas resizing, and exercise resolvers. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant CredentialUtil
participant WebauthnApiService
participant AlertService
TestSuite->>CredentialUtil: getCredentialWithGracefullyHandlingAuthenticatorIssues(credential)
alt credential is null
CredentialUtil-->>TestSuite: return null
else credential.toJSON throws
CredentialUtil-->>TestSuite: throw InvalidCredentialError
else
CredentialUtil-->>TestSuite: return processed credential
end
TestSuite->>CredentialUtil: addNewPasskey(user)
alt user is undefined
CredentialUtil-->>TestSuite: throw Error
else
CredentialUtil->>WebauthnApiService: getRegistrationOptions()
alt getRegistrationOptions throws
CredentialUtil-->>TestSuite: propagate error
else
CredentialUtil->>window.navigator.credentials: create()
alt returns null
CredentialUtil->>AlertService: error()
CredentialUtil-->>TestSuite: throw Error
else
CredentialUtil-->>TestSuite: return new passkey
end
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (3)
src/main/webapp/app/exercise/statistics/doughnut-chart/doughnut-chart.component.spec.ts (1)
52-61: Explicitly set test pre-conditions to keep it future-proofThe test currently depends on the component’s default
contentType(and other defaults) remaining unchanged. If someone later alters those defaults, this test might fail for reasons unrelated to the fallback logic you actually want to check.
Adding the explicit setup makes the intention crystal-clear and decouples the test from implementation details.@@ - it('should use fallback value when currentAbsolute is undefined and stats are not received', () => { - component.currentAbsolute = undefined; - component.receivedStats = false; - component.ngOnChanges(); + it('should use fallback value when currentAbsolute is undefined and stats are not received', () => { + component.contentType = DoughnutChartType.AVERAGE_EXERCISE_SCORE; // ensure deterministic context + component.currentAbsolute = undefined; + component.currentPercentage = undefined; // align both value sources + component.receivedStats = false; + component.ngOnChanges();This small tweak strengthens the test’s resilience without changing its semantics.
src/main/webapp/app/text/manage/text-exercise/service/text-exercise-resolver.service.spec.ts (1)
21-27: Redundant HTTP providers clutter the testing module
provideHttpClientTesting()already suppliesHttpClient. InjectingprovideHttpClient()in addition is unnecessary and can mask mis-configurations if one of them changes.
RemoveprovideHttpClient()for a leaner and less error-prone setup.src/main/webapp/app/modeling/manage/services/modeling-exercise-resolver.service.spec.ts (1)
22-28: Duplicate HTTP providersSame comment as for the TextExercise resolver spec –
provideHttpClient()is redundant whenprovideHttpClientTesting()is present.- provideHttpClient(), provideHttpClientTesting(),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/webapp/app/core/user/settings/passkey-settings/util/credential.util.spec.ts(1 hunks)src/main/webapp/app/exercise/statistics/doughnut-chart/doughnut-chart.component.spec.ts(1 hunks)src/main/webapp/app/modeling/manage/services/modeling-exercise-resolver.service.spec.ts(1 hunks)src/main/webapp/app/shared/image-cropper/utils/exif.utils.spec.ts(1 hunks)src/main/webapp/app/shared/image-cropper/utils/resize.utils.spec.ts(1 hunks)src/main/webapp/app/text/manage/text-exercise/service/text-exercise-resolver.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/shared/image-cropper/utils/exif.utils.spec.tssrc/main/webapp/app/shared/image-cropper/utils/resize.utils.spec.tssrc/main/webapp/app/exercise/statistics/doughnut-chart/doughnut-chart.component.spec.tssrc/main/webapp/app/core/user/settings/passkey-settings/util/credential.util.spec.tssrc/main/webapp/app/text/manage/text-exercise/service/text-exercise-resolver.service.spec.tssrc/main/webapp/app/modeling/manage/services/modeling-exercise-resolver.service.spec.ts
🧬 Code Graph Analysis (1)
src/main/webapp/app/modeling/manage/services/modeling-exercise-resolver.service.spec.ts (1)
src/main/webapp/app/core/course/shared/entities/course.model.ts (1)
Course(59-180)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: client-style
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Mend Security Check
🔇 Additional comments (2)
src/main/webapp/app/shared/image-cropper/utils/resize.utils.spec.ts (1)
27-33:✅ Verification successful
Canvas context is assumed to exist – will fail in plain JSDOM environments
expect(context).not.toBeNull();requires thatHTMLCanvasElement#getContext('2d')returns a working mock.
In the default Jest + JSDOM setup this call returnsnullunlessjest-canvas-mock(or a similar stub) is installed and imported in the Jest setup file.
Without that dependency the assertion will always fail on CI.Consider adding the mock explicitly or replace the expectation with a defensive check that only runs when a context is available.
I’ll verify whether
jest-canvas-mock(or a similar stub) is configured in the repo’s Jest setup:
🏁 Script executed:
#!/bin/bash # Search for jest-canvas-mock references in code/dependencies rg "jest-canvas-mock" . # Check Jest config for setupFiles or setupFilesAfterEnv entries rg "setupFiles" -n . rg "setupFilesAfterEnv" -n .Length of output: 845
jest-canvas-mock is already configured in Jest setup
I verified that:
jest-canvas-mockis listed in package.json and imported in src/test/javascript/spec/jest-test-setup.ts- That setup file is included under setupFilesAfterEnv in jest.config.js
The 2D canvas context will be mocked correctly, so the existing
expect(context).not.toBeNull()check will pass on CI. No changes required.src/main/webapp/app/shared/image-cropper/utils/exif.utils.spec.ts (1)
17-18:❓ Verification inconclusive
Mocking
window.Imageviajest.spyOnis brittle
window.Imageis a constructor function but can be non-configurable in JSDOM.jest.spyOn(window, 'Image')may therefore throw or silently fail, leaving the real constructor in place and breaking the test.A safer pattern:
- jest.spyOn(window, 'Image').mockImplementation(() => mockImage); + const OriginalImage = window.Image; + Object.defineProperty(window, 'Image', { writable: true, value: jest.fn(() => mockImage) }); + // …after the test + Object.defineProperty(window, 'Image', { writable: true, value: OriginalImage });or move this logic to a shared Jest setup file.
Also applies to: 34-35
🏁 Script executed:
#!/bin/bash # Search for usages of jest.spyOn on window.Image in the repository grep -R "jest\.spyOn(window, 'Image')" -n .Length of output: 347
Verify
window.ImageConfigurability Before MockingPlease confirm whether
window.Imageis configurable in your Jest/JSDOM environment. If it’s non-configurable,jest.spyOn(window, 'Image')can fail or be a no-op.• File:
src/main/webapp/app/shared/image-cropper/utils/exif.utils.spec.ts
– Lines 17–18
– Lines 34–35Suggested pattern when
window.Imageis non-configurable:- jest.spyOn(window, 'Image').mockImplementation(() => mockImage); + // Capture original constructor + const OriginalImage = window.Image; + // Override with a writable mock + Object.defineProperty(window, 'Image', { + writable: true, + configurable: true, + value: jest.fn(() => mockImage), + }); + // …run your test… + // Restore original constructor + Object.defineProperty(window, 'Image', { + writable: true, + configurable: true, + value: OriginalImage, + });Alternatively, centralize this setup/teardown in a shared Jest setup file.
src/main/webapp/app/core/user/settings/passkey-settings/util/credential.util.spec.ts
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
Checklist
General
Client
Motivation and Context
I lowered the client test thresholds in #10979, as the touched components are well tested, and it would be an imbalanced effort to increase the coverage within those components; I decided to increase the coverage at other places.
So we keep moving towards the 90% coverage goal and do not make steps back there.
Description
Adds several tests for insufficiently covered parts of the client code.
Steps for Testing
Verify that the tests pass in the pipeline
Review Progress
Performance Review
Code Review
Test Coverage
Summary by CodeRabbit