-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Development
: Fix client test coverage
#9703
Conversation
WalkthroughThis pull request introduces new test suites for the Changes
Suggested labels
Suggested reviewers
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/test/javascript/spec/util/shared/request.util.spec.ts (4)
5-12
: Consider adding edge cases for nested key creation.While the basic nested key functionality is well tested, consider adding test cases for:
- Nested objects with multiple levels
- Null/undefined values in the request object
- Special characters in keys
it('should handle complex nested objects', () => { const req = { key1: { nested: 'value1' }, key2: null, 'special.key': 'value2' }; const parentKey = 'parent'; const params = createNestedRequestOption(req, parentKey); expect(params.get('parent.key1.nested')).toBe('value1'); expect(params.get('parent.key2')).toBeNull(); expect(params.get('parent.special.key')).toBe('value2'); });
22-28
: Add test cases for edge cases in sort parameters.Consider adding test cases for:
- Empty array
- Array with undefined/null values
- Single value array
it('should handle edge cases in sort parameters', () => { const emptyReq = { sort: [] }; const singleReq = { sort: ['value1'] }; const nullReq = { sort: [null, undefined] }; const emptyParams = createNestedRequestOption(emptyReq); const singleParams = createNestedRequestOption(singleReq); const nullParams = createNestedRequestOption(nullReq); expect(emptyParams.getAll('sort')).toEqual([]); expect(singleParams.getAll('sort')).toEqual(['value1']); expect(nullParams.getAll('sort')).toEqual([null, null]); });
30-34
: Use more specific assertions for empty request case.Instead of checking just the length, consider using more specific assertions to validate the empty state.
it('should handle empty request object', () => { const params: HttpParams = createNestedRequestOption(); - expect(params.keys()).toHaveLength(0); + expect(params instanceof HttpParams).toBeTrue(); + expect(params.keys()).toEqual([]); + expect(params.toString()).toBe(''); });
4-35
: Well-structured test suite with room for enhancement.The test suite provides good coverage of the main functionality. Consider organizing the tests into logical groups using
describe
blocks for better readability and maintenance:describe('createNestedRequestOption', () => { describe('basic functionality', () => { // existing nested keys and direct access tests }); describe('sort parameters', () => { // existing and suggested sort parameter tests }); describe('edge cases', () => { // empty request and suggested edge case tests }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/util/shared/request.util.spec.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/util/shared/request.util.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}}
🔇 Additional comments (1)
src/test/javascript/spec/util/shared/request.util.spec.ts (1)
14-20
: LGTM! Clear and focused test case.
The test effectively validates the direct parameter creation without nesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/util/shared/utils.spec.ts
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/util/shared/utils.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}}
🔇 Additional comments (1)
src/test/javascript/spec/util/shared/utils.spec.ts (1)
14-14
: LGTM!
The import statement follows the established pattern and best practices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New test lgtm
Checklist
General
Client
Motivation and Context
Client tests are failing on develop right now https://bamboo.ase.in.tum.de/browse/ARTEMIS-TESTS-TSTEST-6455
When merging develop into Development: Use signals in video unit form component #9692.
After adding tests for a 100% coverage in the touched component of the mentioned PR and a still failing coverage condition I realized that the issue arises from develop.
Description
This PR fixes the client coverage on develop.
Steps for Testing
Verify the client tests do not fail for this PR.
Review Progress
Code Review
Summary by CodeRabbit
Range
class, verifying string representations for specified ranges.