fix: address test error handling in password policy tests #1616
Conversation
- Remove unused PostgrestError import - Fix finally block to preserve test errors - Add proper error validation for RPC call Co-authored-by: riderx <4084527+riderx@users.noreply.github.com>
… copilot/sub-pr-1614
There was a problem hiding this comment.
Pull request overview
Updates the password policy test suite to avoid masking failures during cleanup and to validate production RPC paths more strictly, improving reliability of backend integration tests.
Changes:
- Removed an unused
PostgrestErrortype import. - Refactored a test to preserve assertion failures while still performing cleanup in
finally. - Added explicit validation of
get_password_policy_hashRPC results (fail fast on RPC errors / null results).
| finally { | ||
| const { error } = await getSupabaseClient() | ||
| const { error: restoreError } = await getSupabaseClient() | ||
| .from('orgs') | ||
| .update({ password_policy_config: policyConfig }) | ||
| .eq('id', ORG_ID) | ||
| if (error) | ||
| throw error | ||
|
|
||
| // If restore failed, throw it (but preserve test failure if there was one) | ||
| if (restoreError) { | ||
| if (testError) | ||
| throw new Error(`Test failed AND restore failed: ${testError.message} | Restore error: ${restoreError.message}`) | ||
| throw restoreError |
There was a problem hiding this comment.
The finally block can still mask the original test failure if the restore update(...) call rejects/throws (network/client exception) before restoreError is checked. Consider wrapping the restore operation in its own try/catch and, when both the test and restore fail, throw an AggregateError (or use Error with cause) so the original assertion stack trace is preserved instead of replacing it with a new Error built from messages.
| expect(orgError).toBeNull() | ||
| expect(org?.password_policy_config).not.toBeNull() | ||
|
|
||
| // Use the same RPC that production uses to compute the password policy hash | ||
| const { data: rpcResult } = await getSupabaseClient().rpc('get_password_policy_hash', { | ||
| const { data: rpcResult, error: rpcError } = await getSupabaseClient().rpc('get_password_policy_hash', { | ||
| policy_config: org?.password_policy_config, | ||
| }) | ||
|
|
||
| const policyHash = (rpcResult as string | null) ?? 'test_hash' | ||
| expect(rpcError).toBeNull() | ||
| expect(rpcResult).not.toBeNull() | ||
| expect(typeof rpcResult).toBe('string') |
There was a problem hiding this comment.
expect(org?.password_policy_config).not.toBeNull() and expect(rpcResult).not.toBeNull() will still pass when the value is undefined (because undefined !== null). Use an assertion that also rejects undefined (e.g., assert org is truthy and then access org.password_policy_config, and assert rpcResult is defined/string) so the test fails at the right place with a clear signal.
|
|
||
| // If restore failed, throw it (but preserve test failure if there was one) | ||
| if (restoreError) { | ||
| if (testError) | ||
| throw new Error(`Test failed AND restore failed: ${testError.message} | Restore error: ${restoreError.message}`) | ||
| throw restoreError | ||
| } | ||
|
|
There was a problem hiding this comment.
There are blank lines containing trailing whitespace (e.g., around the restore block). Please remove the trailing spaces to avoid formatter/lint churn in future diffs.
| // If restore failed, throw it (but preserve test failure if there was one) | |
| if (restoreError) { | |
| if (testError) | |
| throw new Error(`Test failed AND restore failed: ${testError.message} | Restore error: ${restoreError.message}`) | |
| throw restoreError | |
| } | |
| // If restore failed, throw it (but preserve test failure if there was one) | |
| if (restoreError) { | |
| if (testError) | |
| throw new Error(`Test failed AND restore failed: ${testError.message} | Restore error: ${restoreError.message}`) | |
| throw restoreError | |
| } |
|
Summary
Fixed three error handling issues in password policy tests identified by code review that could mask test failures or hide broken production paths.
Changes:
PostgrestErrortype no longer referenced after refactoringget_password_policy_hasherrors now fail tests instead of silently falling back to dummy hashTest plan
Screenshots
N/A - backend test changes only
Checklist
bun run lint:backend && bun run lint.accordingly.
my tests
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.