Skip to content

Fixed event comparison issue in Syncpointspec tests #6786

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

Merged
merged 7 commits into from
Nov 15, 2022

Conversation

maneesht
Copy link
Contributor

@maneesht maneesht commented Nov 12, 2022

Fixes problem where comparisons between expected and actual events weren't happening correctly.

Fixing this caused other tests to fail because we expected specific events to be raised before other events. However, this is not how the SDKs work now. They order events based on where the event is in the synctree, not based on type of the event.

@maneesht maneesht requested review from jsdt and jmwski as code owners November 12, 2022 00:51
@changeset-bot
Copy link

changeset-bot bot commented Nov 12, 2022

🦋 Changeset detected

Latest commit: 888a98d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

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

Not sure what this means? Click here to learn what changesets are.

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

@github-actions
Copy link
Contributor

github-actions bot commented Nov 12, 2022

Changeset File Check ⚠️

  • Warning: This PR modifies files in the following packages but they have not been included in the changeset file:

    • @firebase/database

    Make sure this was intentional.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Nov 12, 2022

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Nov 12, 2022

@maneesht maneesht requested a review from egilmorez as a code owner November 14, 2022 02:42
Comment on lines 232 to 238
const EVENT_ORDERING = [
'child_removed',
'child_added',
'child_moved',
'child_changed',
'value'
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@puf It seems that at one point we were enforcing this order. Just FYI.

@jmwski jmwski assigned maneesht and unassigned jmwski Nov 14, 2022
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as formality (no doc impact). Thanks!

@maneesht maneesht merged commit a9add5e into master Nov 15, 2022
@maneesht maneesht deleted the mtewani/fix-syncpoint-test-compare branch November 15, 2022 16:21
levino pushed a commit to levino/firebase-js-sdk that referenced this pull request Dec 10, 2022
@firebase firebase locked and limited conversation to collaborators Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants