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

Port Compat tests to v9 API #5844

Merged
merged 11 commits into from Jan 12, 2022
Merged

Port Compat tests to v9 API #5844

merged 11 commits into from Jan 12, 2022

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Dec 30, 2021

We currently use the Compat API to test v9. This works because every API in v9 is also available in Compat. As we are adding new APIs, this will stop working though. This PR migrates both the Firestore integration tests and the minified tests to use the v9 API.

There is another PR pending that re-adds the Compat tests: #5870

@changeset-bot
Copy link

changeset-bot bot commented Dec 30, 2021

⚠️ No Changeset found

Latest commit: 53d5a73

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 Dec 30, 2021

Size Report 1

Affected Products

  • @firebase/firestore

    TypeBase (d0c8bfe)Merge (fff74fb)Diff
    browser228 kB228 kB-80 B (-0.0%)
    esm5285 kB285 kB-80 B (-0.0%)
    main427 kB426 kB-83 B (-0.0%)
    module228 kB228 kB-80 B (-0.0%)
    react-native228 kB228 kB-80 B (-0.0%)
  • @firebase/firestore-lite

    TypeBase (d0c8bfe)Merge (fff74fb)Diff
    browser72.6 kB72.5 kB-80 B (-0.1%)
    esm585.9 kB85.8 kB-80 B (-0.1%)
    main125 kB125 kB-83 B (-0.1%)
    module72.6 kB72.5 kB-80 B (-0.1%)
    react-native72.8 kB72.7 kB-80 B (-0.1%)
  • @firebase/messaging

    TypeBase (d0c8bfe)Merge (fff74fb)Diff
    browser20.8 kB20.8 kB-2 B (-0.0%)
    esm526.1 kB26.1 kB-2 B (-0.0%)
    main26.8 kB26.8 kB-2 B (-0.0%)
    module20.8 kB20.8 kB-2 B (-0.0%)
  • @firebase/messaging-sw

    TypeBase (d0c8bfe)Merge (fff74fb)Diff
    module22.8 kB22.8 kB-6 B (-0.0%)
  • bundle

    TypeBase (d0c8bfe)Merge (fff74fb)Diff
    firestore (Query Cursors)189 kB189 kB-20 B (-0.0%)
    firestore (Query)190 kB190 kB-50 B (-0.0%)
    firestore (Transaction)163 kB163 kB-10 B (-0.0%)
    firestore (Write data)162 kB162 kB-10 B (-0.0%)
    firestore-lite (Query Cursors)56.4 kB56.4 kB-20 B (-0.0%)
    firestore-lite (Query)59.5 kB59.5 kB-50 B (-0.1%)
    firestore-lite (Transaction)61.4 kB61.3 kB-10 B (-0.0%)
    firestore-lite (Write data)46.9 kB46.9 kB-10 B (-0.0%)
    messaging (send + receive)37.8 kB37.8 kB-2 B (-0.0%)
  • firebase

    TypeBase (d0c8bfe)Merge (fff74fb)Diff
    firebase-compat.js753 kB753 kB-88 B (-0.0%)
    firebase-firestore-compat.js281 kB281 kB-80 B (-0.0%)
    firebase-firestore-lite.js249 kB249 kB-86 B (-0.0%)
    firebase-firestore.js770 kB770 kB-119 B (-0.0%)
    firebase-messaging-compat.js38.0 kB37.9 kB-8 B (-0.0%)
    firebase-messaging-sw.js102 kB102 kB-6 B (-0.0%)
    firebase-messaging.js101 kB101 kB-2 B (-0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/9uCrskjXgN.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Dec 30, 2021

Size Analysis Report 1

This report is too large (322,844 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/fSMrc6aJ90.html

@schmidt-sebastian schmidt-sebastian marked this pull request as ready for review January 10, 2022 19:10
Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

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

13 / 45 files reviewed so far...

} else {
process.env.INCLUDE_FIRESTORE_PERSISTENCE = '${isPersistenceEnabled()}';
}`
if (typeof process === 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this indentation change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the output looks much better. This is a search/replace thing and the output shouldn't be indented. It doesn't really matter though since no one should see this, but I like to go above and beyond :)

{ includeMetadataChanges: true },
accumulator.storeEvent
);

// wait for initial null snapshot to avoid potential races.
const snapshot = await accumulator.awaitRemoteEvent();
expect(snapshot.exists).to.be.false;
expect(snapshot.exists()).to.be.false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch. Nothing about this PR, but this is such an easy thing to miss. @cherylEnkidu spent a lot of time debugging her code as she had used snapshot.exists while using v9 :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

TAT People use exists in v8 but exists() in v9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We changed it to a method so we could use a typeguard:

exists(): this is QueryDocumentSnapshot<T>;

If exists() returns true then TS knows .data is not undefined.

Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

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

25 / 45 ...

import { apiDescribe, withTestDb } from '../util/helpers';
import { asyncQueue } from '../util/internal_helpers';

apiDescribe('Idle Timeout', (persistence: boolean) => {
it('can write document after idle timeout', () => {
it('can write documdent after idle timeout', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@schmidt-sebastian schmidt-sebastian merged commit 3105541 into master Jan 12, 2022
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/newtests branch January 12, 2022 21:59
@firebase firebase locked and limited conversation to collaborators Feb 12, 2022
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.

None yet

4 participants