test: Create tool for mocking event listeners #14415
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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
CodeRabbit Configuration File (
|
github-actions
bot
added
area/process
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14415 +/- ##
=======================================
Coverage 95.65% 95.65%
=======================================
Files 1885 1886 +1
Lines 24495 24516 +21
Branches 2812 2816 +4
=======================================
+ Hits 23431 23452 +21
Misses 804 804
Partials 260 260 ☔ View full report in Codecov by Sentry. |
TomasEng
added
quality/testing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
frontend/packages/process-editor/test/EventListeners.ts (1)
31-41: Add error handling for invalid inputs.The
removemethod silently ignores attempts to remove non-existent listeners. Consider adding validation and error handling:remove(eventName: string, callback: Function): void { if (this.has(eventName)) { + const currentList = this.get(eventName); + if (!currentList.includes(callback)) { + console.warn( + `Attempted to remove non-existent listener for event "${eventName}"` + ); + return; + } this.removeListener(eventName, callback); } }🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 37-37: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 39-39: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
frontend/packages/process-editor/test/EventListeners.test.ts (2)
3-45: Add tests for edge cases and error handling.The test suite is comprehensive for happy paths but could benefit from additional test cases:
- Test removing a non-existent listener
- Test memory leak scenarios (add/remove many listeners)
- Test type safety with the proposed generic implementation
Example test case:
it('Warns when removing non-existent listener', () => { const eventListeners = new EventListeners(); const fun = jest.fn(); const eventName = 'event'; const consoleSpy = jest.spyOn(console, 'warn'); eventListeners.remove(eventName, fun); expect(consoleSpy).toHaveBeenCalledWith( 'Attempted to remove non-existent listener for event "event"' ); });
81-140: Improve test organization with shared setup.Consider extracting common setup code into beforeEach blocks to reduce duplication:
describe('triggerEvent', () => { let eventListeners: EventListeners; let mockFn: jest.Mock; beforeEach(() => { eventListeners = new EventListeners(); mockFn = jest.fn(); }); // ... existing tests using eventListeners and mockFn });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/packages/process-editor/test/EventListeners.test.ts(1 hunks)frontend/packages/process-editor/test/EventListeners.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/packages/process-editor/test/EventListeners.ts
[error] 56-56: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 4-4: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 7-7: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 17-17: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 22-22: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 27-27: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 31-31: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 37-37: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 39-39: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 51-51: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 51-51: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/packages/process-editor/test/EventListeners.ts (3)
3-6: Consider making the inner array type more specific.The current type allows mixing different event handler types in the array. Consider using
Array<EventMap[K]>to ensure type consistency within each event's handlers.type ListenerMap<EventMap extends Record<string, (...args: any[]) => void>> = Map< keyof EventMap, - Array<EventMap[keyof EventMap]> + Array<EventMap[K]> >;
15-23: Consider adding error handling for non-existent events.The
triggerEventmethod silently ignores non-existent events. Consider adding an optional parameter to control error handling behavior.triggerEvent<Key extends keyof EventMap>( eventName: Key, + options?: { throwOnMissing?: boolean }, ...params: Parameters<EventMap[Key]> ): void { if (this.has(eventName)) { const functions = this.get(eventName); functions.forEach((fun) => fun(...params)); + } else if (options?.throwOnMissing) { + throw new Error(`No listeners registered for event: ${String(eventName)}`); } }
25-28: Add duplicate check to prevent multiple identical listeners.The
addmethod doesn't check for duplicate listeners, which could lead to the same callback being executed multiple times.add<Key extends keyof EventMap>(eventName: Key, callback: EventMap[Key]): void { + if (this.functionExists(eventName, callback)) return; if (this.has(eventName)) this.addListenerToCurrentList<Key>(eventName, callback); else this.createNewListenerList<Key>(eventName, [callback]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/packages/process-editor/test/EventListeners.test.ts(1 hunks)frontend/packages/process-editor/test/EventListeners.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/process-editor/test/EventListeners.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Testing
🔇 Additional comments (2)
frontend/packages/process-editor/test/EventListeners.ts (2)
8-13: Well-structured class declaration with proper type safety!The class is well-designed with:
- Generic type constraints for type safety
- Readonly modifier to prevent accidental list reassignment
- Clean constructor initialization
1-87: Excellent implementation of the event listener system!The implementation is well-structured with:
- Strong type safety through generics
- Clear separation of concerns
- Good error handling
- Clean and maintainable code organization
This aligns perfectly with the PR objective of creating a tool for mocking event listeners in tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/packages/process-editor/test/EventListeners.ts (5)
3-6: Consider constraining event parameter types.Good use of generics and type alias. Consider tightening the constraint on event parameters:
-type ListenerMap<EventMap extends Record<string, (...args: any[]) => void>> = Map< +type ListenerMap<EventMap extends Record<string, (...args: unknown[]) => void>> = Map< keyof EventMap, Array<EventMap[keyof EventMap]> >;
25-43: Consider simplifying the event addition logic.The current implementation splits the logic into multiple methods, which adds unnecessary complexity.
add<Key extends keyof EventMap>(eventName: Key, callback: EventMap[Key]): void { - if (this.has(eventName)) this.addListenerToCurrentList<Key>(eventName, callback); - else this.createNewListenerList<Key>(eventName, [callback]); + const currentListeners = this.get<Key>(eventName) ?? []; + this.set<Key>(eventName, [...currentListeners, callback]); } - - private addListenerToCurrentList<Key extends keyof EventMap>( - eventName: Key, - callback: EventMap[Key], - ): void { - const currentListeners = this.get<Key>(eventName); - this.set<Key>(eventName, [...currentListeners, callback]); - } - - private createNewListenerList<Key extends keyof EventMap>( - eventName: Key, - callbacks: EventMap[Key][], - ): void { - this.set<Key>(eventName, callbacks); - }
47-49: Simplify error message construction.The
toString()call is unnecessary as template literals automatically convert values to strings.throw new Error( - `The provided callback function does not exist on the ${eventName.toString()} listener.`, + `The provided callback function does not exist on the ${eventName} listener.`, );
45-67: Consider combining function existence check with removal.The current implementation checks for existence twice (in
functionExistsandget).remove<Key extends keyof EventMap>(eventName: Key, callback: EventMap[Key]): void { - if (!this.functionExists<Key>(eventName, callback)) + const currentList = this.get<Key>(eventName); + if (!currentList?.includes(callback)) throw new Error( `The provided callback function does not exist on the ${eventName} listener.`, ); - this.removeListener<Key>(eventName, callback); + this.set<Key>(eventName, ArrayUtils.removeItemByValue(currentList, callback)); }
77-82: Simplify set method signature.The method doesn't need to return the Map instance since it's not used.
private set<Key extends keyof EventMap>( eventName: Key, callbacks: EventMap[Key][], - ): ListenerMap<EventMap> { - return this.list.set(eventName, callbacks); + ): void { + this.list.set(eventName, callbacks); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/process-editor/test/EventListeners.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
frontend/packages/process-editor/test/EventListeners.ts (1)
Learnt from: TomasEng
PR: Altinn/altinn-studio#14415
File: frontend/packages/process-editor/test/EventListeners.ts:73-75
Timestamp: 2025-01-14T12:03:39.929Z
Learning: In the EventListeners class, the `get` method is only called after verifying key existence with `has`, making undefined checks unnecessary and the type assertion safe.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (2)
frontend/packages/process-editor/test/EventListeners.ts (2)
15-23: Well-implemented event triggering logic!The implementation shows excellent type safety and proper parameter handling.
1-87: Overall excellent implementation!The EventListeners class provides a robust, type-safe implementation for managing event listeners. The code is well-structured, properly encapsulated, and includes appropriate error handling. While there are some minor opportunities for refinement, the implementation is solid and suitable for its intended use in testing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/process-editor/test/EventListeners.ts (2)
45-51: Consider improving error message formatting.The error message uses string concatenation with type assertion. Consider using template literals for better readability.
- `The provided callback function does not exist on the ${eventName as string} listener.`, + `The provided callback function does not exist on the '${String(eventName)}' listener.`,
77-82: Consider removing unnecessary return.The
setmethod returns the Map instance which isn't used. Consider making it void.private set<Key extends keyof EventMap>( eventName: Key, callbacks: EventMap[Key][], - ): ListenerMap<EventMap> { - return this.list.set(eventName, callbacks); + ): void { + this.list.set(eventName, callbacks); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/process-editor/test/EventListeners.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
frontend/packages/process-editor/test/EventListeners.ts (1)
Learnt from: TomasEng
PR: Altinn/altinn-studio#14415
File: frontend/packages/process-editor/test/EventListeners.ts:73-75
Timestamp: 2025-01-14T12:03:39.929Z
Learning: In the EventListeners class, the `get` method is only called after verifying key existence with `has`, making undefined checks unnecessary and the type assertion safe.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Testing
🔇 Additional comments (5)
frontend/packages/process-editor/test/EventListeners.ts (5)
1-7: LGTM! Well-structured type definitions.The generic type
ListenerMapprovides a robust and type-safe foundation for the event listener implementation.
8-13: LGTM! Type-safe class implementation.The generic class declaration with proper type constraints and minimal constructor is well-implemented.
15-23: LGTM! Well-implemented event triggering.The method correctly handles type safety and ensures events exist before triggering.
25-28: LGTM! Clean branching logic.The method efficiently handles both new and existing event listeners.
1-87: Verify test coverage for edge cases.The implementation aligns well with the PR objectives for mocking event listeners. Consider adding tests for the following scenarios to ensure robust mocking:
- Removing the last listener for an event
- Adding duplicate listeners
- Concurrent event triggering
Would you like me to help generate these test cases?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/process-editor/test/EventListeners.ts (1)
25-28: Consider consistent error handling across methods.The
removemethod throws an error for non-existent listeners whileaddsilently creates new lists. Consider making the error handling behavior consistent across these methods. Options:
- Make
addthrow an error for invalid event names (matchingremove's strictness)- Make
removesilently ignore non-existent listeners (matchingadd's flexibility)The choice depends on your error handling philosophy:
- Option 1 provides stronger guarantees but requires more error handling by callers
- Option 2 is more flexible but might hide programming errors
Also applies to: 45-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/process-editor/test/EventListeners.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
frontend/packages/process-editor/test/EventListeners.ts (1)
Learnt from: TomasEng
PR: Altinn/altinn-studio#14415
File: frontend/packages/process-editor/test/EventListeners.ts:73-75
Timestamp: 2025-01-14T12:03:39.929Z
Learning: In the EventListeners class, the `get` method is only called after verifying key existence with `has`, making undefined checks unnecessary and the type assertion safe.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (6)
frontend/packages/process-editor/test/EventListeners.ts (6)
1-6: Well-structured type definitions!The
ListenerMaptype definition provides excellent type safety through generic constraints and proper typing of the event listener map.
8-13: Clean class setup with proper encapsulation!The class is well-structured with appropriate generic constraints and readonly protection for the internal list.
15-23: Type-safe event triggering implementation!The
triggerEventmethod provides excellent type safety through generic constraints while maintaining runtime safety with existence checks.
25-28: Clean and efficient listener addition logic!The
addmethod handles both cases (existing and new listeners) elegantly with proper type safety.
45-51: Robust error handling in remove implementation!The
removemethod properly validates listener existence before removal and provides a descriptive error message.
81-83: Simple and effective clear implementation!The
clearmethod efficiently removes all listeners.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/process-editor/test/EventListeners.ts (1)
45-54: Consider adding JSDoc comments for better documentation.The error handling is good, but adding JSDoc comments would help users understand:
- The expected parameters
- When and why errors might be thrown
- Example usage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/process-editor/test/EventListeners.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
frontend/packages/process-editor/test/EventListeners.ts (1)
Learnt from: TomasEng
PR: Altinn/altinn-studio#14415
File: frontend/packages/process-editor/test/EventListeners.ts:73-75
Timestamp: 2025-01-14T12:03:39.929Z
Learning: In the EventListeners class, the `get` method is only called after verifying key existence with `has`, making undefined checks unnecessary and the type assertion safe.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
- GitHub Check: Typechecking and linting
🔇 Additional comments (4)
frontend/packages/process-editor/test/EventListeners.ts (4)
1-6: Well-structured type definitions with proper type safety!The
ListenerMaptype effectively uses generics to ensure type safety while maintaining flexibility. The use ofunknown[]for function parameters is a good practice as it's safer thanany[].
8-13: Clean class setup with proper immutability!Good practices observed:
- Generic type parameter with appropriate constraints
- Readonly list property prevents accidental mutations
- Minimal constructor that properly initializes the map
15-23: Consider adding error handling for invalid parameters.While the method is well-implemented, consider adding parameter validation to handle edge cases:
- Null/undefined event name
- Invalid parameter types that don't match the event signature
30-73: Well-structured private methods with proper encapsulation!The private helper methods are well-organized and maintain good separation of concerns. The type assertion in the
getmethod is safe as it's only called after verifying existence withhas.
Description
This pull request adds a tool for mocking event listeners in tests. It provides methods for storing and triggering custom event listeners. It is made specifically to mock the event listeners of the BPMN modeler tool, but it is quite generic and may also be used for other use cases if necessary.
Background
The tests of the
useBpmnEditorhook are cumbersome to read and contain a lot of mocks that rely strictly on implementation details, so I am trying to clean that up. The mock of thebpmn-js/lib/Modelerpackage is necessary because it uses tools that are not supported by JSDOM, but the current mock does not mimic event listeners correctly.How to use
In the test suite, the modeler tool may be mocked like this:
This code creates an event listener instance and applies its
addandremovemethods to the corresponding methods of the BPMN modeler object.We may now use the
triggerEventmethod to simulate triggering events and theclearmethod to clear the event listeners between each test:Note
This pull request does not implement the actual usage of the tool. That will be done in a future pull request.
Related Issue
Verification
Summary by CodeRabbit
Summary by CodeRabbit
New Features
EventListenersclass for managing event listeners.Tests
EventListenersclass.