Fix dead-code OR expression in permission-denied assertion#443
Fix dead-code OR expression in permission-denied assertion#443swseverance wants to merge 1 commit into
Conversation
`'permission-denied' || 'PERMISSION_DENIED'` always evaluates to the first string due to JS short-circuit; drop the unreachable alternative so the assertion matches only the actual error code.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the permission-denied error code checks in test utility files by removing a logical OR operation that was previously ineffective. The reviewer suggests that if the original intent was to support multiple error code formats across different environments, an array-based check using toContain should be implemented instead of a single string comparison.
| export async function expectFirestorePermissionDenied(promise: Promise<any>) { | ||
| const errorResult = await assertFails(promise); | ||
| expect(errorResult.code).toBe('permission-denied' || 'PERMISSION_DENIED'); | ||
| expect(errorResult.code).toBe('permission-denied'); |
There was a problem hiding this comment.
The original code 'permission-denied' || 'PERMISSION_DENIED' was a logical error in JavaScript/TypeScript that always evaluated to the first string. While removing the dead code is an improvement, if the intention was to support both possible error codes (e.g., to handle variations across different SDK versions or environments), you should use an array check with toContain instead of a single string comparison.
| expect(errorResult.code).toBe('permission-denied'); | |
| expect(['permission-denied', 'PERMISSION_DENIED']).toContain(errorResult.code); |
| export async function expectFirestorePermissionDenied(promise: Promise<any>) { | ||
| const errorResult = await assertFails(promise); | ||
| expect(errorResult.code).toBe('permission-denied' || 'PERMISSION_DENIED'); | ||
| expect(errorResult.code).toBe('permission-denied'); |
There was a problem hiding this comment.
The original code 'permission-denied' || 'PERMISSION_DENIED' was a logical error in JavaScript/TypeScript that always evaluated to the first string. While removing the dead code is an improvement, if the intention was to support both possible error codes (e.g., to handle variations across different SDK versions or environments), you should use an array check with toContain instead of a single string comparison.
| expect(errorResult.code).toBe('permission-denied'); | |
| expect(['permission-denied', 'PERMISSION_DENIED']).toContain(errorResult.code); |
1 participant
'permission-denied' || 'PERMISSION_DENIED'always evaluates to the first string due to JS short-circuit; drop the unreachable alternative so the assertion matches only the actual error code.