Skip to content
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

Firestore: equality_matcher.ts: fix missing custom assertion failure message in deep equals #7519

Merged
merged 3 commits into from
Aug 3, 2023

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Aug 2, 2023

This PR fixes a bug in Firestore's unit/integration testing framework where a "deep equals" assertion check would omit the custom failure message, when it should have included the custom failure message.

For example, consider the following "expect" checks:

expect({foo: 42}, 'custom message').to.deep.equal({bar: 42});
expect({foo: 42}).to.deep.equal({bar: 42}, 'custom message');

Both of these checks will unconditionally fail because {foo: 42} does not "deep equal" {bar: 42}. Also, both the checks specify a custom failure message to be incorporated into the failure message, albeit in different ways.

The problem is that the custom message is not included in the ultimate failure message:

AssertionError: expected { foo: 42 } to roughly deeply equal { bar: 42 }

With the fix in this PR, the custom message is included in the failure message:

AssertionError: custom message: expected { foo: 42 } to roughly deeply equal { bar: 42 }

The root cause of the bug was the misconception that custom message would be specified as an argument to the function. The function signature was actually incorrectly typed as (r: unknown, l: unknown) => boolean, suggesting that it was supposed to compare r and l for equality. This coincidentally just happened to work because r was treated as the "expected" and the l argument was simply ignored, which worked because it was always undefined anyways. The type was fixed to (expected: unknown) => void.

The custom function also did not need to call utils.flag(this, 'message', msg) because the chai testing framework already makes this call, and when the custom function made the call it just clobbered the previously-set custom message with undefined.

@dconeybe dconeybe self-assigned this Aug 2, 2023
@changeset-bot
Copy link

changeset-bot bot commented Aug 2, 2023

⚠️ No Changeset found

Latest commit: 659343e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 2, 2023

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 2, 2023

@dconeybe dconeybe marked this pull request as ready for review August 3, 2023 03:16
@dconeybe dconeybe requested review from a team as code owners August 3, 2023 03:16
@wu-hui wu-hui merged commit e201e53 into master Aug 3, 2023
26 checks passed
@wu-hui wu-hui deleted the dconeybe/EqualityMatcherMessageDroppingFix branch August 3, 2023 14:44
@firebase firebase locked and limited conversation to collaborators Sep 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants