Share Integration Tests between firestore-exp and the main SDK #3374

schmidt-sebastian · 2020-07-08T22:52:37Z

This PR removes the custom integration tests for firestore-exp and changes the integration tests under test/integration/api/ to be able to run against the firestore-exp SDK (which I started calling "modular SDK" in the code, but maybe I shouldn't do that).

The logic is based on the FirestoreShim added in #3361

Couple things:

I verified that we didn't lose any test coverage.
I updated two error messages in the firestore-exp SDK to match those in the main SDK.
I added the new tests to the the main test rule in package.json, making sure they run on CI.
I added test runners for IntelliJ.
The update to "firebase_export" is to make sure that the "minified" integration tests continue to work as is.

changeset-bot · 2020-07-08T22:52:40Z

🦋 Changeset is good to go

Latest commit: b6abc27

We got this.

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

google-oss-bot · 2020-07-08T23:02:15Z

Binary Size Report

Affected SDKs

@firebase/firestore/exp

Type Base (a875bbe) Head (9b82225) Diff

main 401 kB 401 kB +184 B (+0.0%)
@firebase/firestore/lite

Type Base (a875bbe) Head (9b82225) Diff

main 125 kB 125 kB +175 B (+0.1%)

Test Logs

Base (a875bbe0): https://github.com/firebase/firebase-js-sdk/actions/runs/172119675
Head (9b82225a): https://github.com/firebase/firebase-js-sdk/actions/runs/172302942

wilhuff

This seems like a lot of plumbing that ultimately serves to make the FirestoreShim stand on equal footing to the classic Firestore implementation. Is it possible to just make the FirestoreShim the actual implementation of classic Firestore? That way running one set of tests would verify both implementations, since the classic version is just a thin veneer over the exp implementation.

It seems like a potential stumbling block would be the difference in the way settings are handled, but it seems like we could handle that by just re-instantiating the underlying exp Firestore on settings change.

wilhuff · 2020-07-10T18:18:19Z

integration/firestore/firebase_export.ts

+  return firestore;
+}
+
+export function usesModularApi(): false {


"Modular" doesn't really cleanly describe the difference between the the Firestore-API-with-classes vs the tree-shakeable one.

The core of the difference seems to be class-based vs function-based. To me this suggests that this should be the basis of this kind of property.

schmidt-sebastian

(No code updates yet).

The future plan for clients that are rewritten from scratch rather than replaced inline was to build a shim layer that implements the new API in terms of the old API. I essentially implemented this for our tests. There are some areas where the Shim does less work (e.g. validation), but it is pretty close to the model that @Feiyang1 had originally proposed for all SDKs.

I can add some of the validation to the Shim layer, which would mean we would run more tests. Even with that, it still feels really scary to me to replace our existing code with the shim (at least at this point in time).

I also have to rewrite all user data as the enter and leave the Shim, which we would need to optimize before our users are using the Shim.

Furthermore, I can reduce the amount of plumbing by not adding the test files to exp/test and instead just changing the test rules. Is that something I should explore?

wilhuff · 2020-07-10T20:17:25Z

(No code updates yet).

The future plan for clients that are rewritten from scratch rather than replaced inline was to build a shim layer that implements the new API in terms of the old API. I essentially implemented this for our tests. There are some areas where the Shim does less work (e.g. validation), but it is pretty close to the model that @Feiyang1 had originally proposed for all SDKs.

Ultimately though, the model that was proposed was that the classic API would delegate to the new exp API. As proposed this PR doesn't do that; instead it leaves two parallel implementations that just happen to be testable via the same integration tests.

I suppose what you're saying right now is that you don't have enough confidence in the exp API to make this swap, and that's fair. If the intent behind this PR is that this arrangement is temporary I'm more OK with it. I still think it's a lot of plumbing though.

I can add some of the validation to the Shim layer, which would mean we would run more tests. Even with that, it still feels really scary to me to replace our existing code with the shim (at least at this point in time).

Isn't this validation logic something we'd need in firestore-exp? If it's not needed there, why is it needed in the classic Firestore?

I also have to rewrite all user data as the enter and leave the Shim, which we would need to optimize before our users are using the Shim.

What is the source of this problem? Aren't things like Timestamps and GeoPoints the same in both implementations?

Furthermore, I can reduce the amount of plumbing by not adding the test files to exp/test and instead just changing the test rules. Is that something I should explore?

Re test rules, that seems reasonable.

Part of my concern though is that test duration in the JS repo is already quite long, so essentially doubling the size of the integration test suite seems painful. Moving to a model where the classic API delegated to the functional API would allow you to test them both together (because the classic API would be the shim).

schmidt-sebastian · 2020-07-10T22:26:26Z

I updated the name of useModularApi to useFunctionalApi and removed the new test files.

Ultimately though, the model that was proposed was that the classic API would delegate to the new exp API. As proposed this PR doesn't do that; instead it leaves two parallel implementations that just happen to be testable via the same integration tests.

I suppose what you're saying right now is that you don't have enough confidence in the exp API to make this swap, and that's fair. If the intent behind this PR is that this arrangement is temporary I'm more OK with it. I still think it's a lot of plumbing though.

The goal of this PR is for now just to share the tests, not the surface. If we did share the surface, we wouldn't need to share the tests and wouldn't have to run them twice (which is something I hadn't considered).
I agree it is a good goal to get into a place where we only have one test run and one API surface, but I would like to do this over a matter of weeks and ideally only transform a small set of API types per release. This allows us to slowly roll out this change and make it small increments, which will be much easier to review. It also gives us production testing of the experimental API.

Isn't this validation logic something we'd need in firestore-exp? If it's not needed there, why is it needed in the classic Firestore?

@Feiyang1 and I have been talking about our input validation, as it adds a lot of overhead to the SDK. At the moment, I have only implemented the kind of validation we have in the other Firestore SDKs, which should prevent invalid state for syntactically valid input. There is an ongoing effort to come to a conclusion on our strategy for Web SDK input validation: go/jscore-input-validation

If we added the input validation to the Shim layer and also implemented timestampsInSnapshots, almost no test changes would be needed. If we want to productionize the Shim layer, that would be a good first step (but it would likely increase the size of this PR, rather than decrease it).

I also have to rewrite all user data as the enter and leave the Shim, which we would need to optimize before our users are using the Shim.
What is the source of this problem? Aren't things like Timestamps and GeoPoints the same in both implementations?

FieldPath, FieldValue, and DocumentReference. FieldPath and FieldValue would be relatively trivial to accept without rewriting, since they are input-only types.

schmidt-sebastian · 2020-07-16T17:06:22Z

PTAL :) Thanks.

wilhuff · 2020-07-16T17:37:12Z

packages/firestore/package.json

    "pregendeps:lite": "yarn build:lite",
    "gendeps:lite": "../../scripts/exp/extract-deps.sh --types ./lite/index.d.ts --bundle ./dist/lite/index.node.esm2017.js --output ./lite/test/dependencies.json",
-    "test:exp": "TS_NODE_CACHE=NO TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' nyc --reporter lcovonly -- mocha 'exp/test/**/*.test.ts' --file exp/index.node.ts --config ../../config/mocharc.node.js",
+    "test:exp": "TS_NODE_CACHE=NO TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' nyc --reporter lcovonly -- mocha 'test/integration/api/*.test.ts' --file exp/index.node.ts --config ../../config/mocharc.node.js",
+    "test:exp:persistence": "USE_MOCK_PERSISTENCE=YES TS_NODE_CACHE=NO TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' nyc --reporter lcovonly -- mocha 'test/integration/api/*.test.ts' --require ts-node/register --require exp/index.node.ts --require test/util/node_persistence.ts  --config ../../config/mocharc.node.js",


These entries are borderline illegible. Is it worth pulling them out into scripts? (Feel free to ignore/defer until later.)

I would say you are being nice here :) There are 100% illegible. I tried to create a script this morning, but I failed so I am going to tackle this later.

wilhuff · 2020-07-16T17:41:51Z

packages/firestore/test/integration/api/database.test.ts

+        // FirestoreShim returns a new instance of FirebaseFirestore that is
+        // semantically identical to the originating instance, but does not
+        // compare equal using the default JavaScript semantics.
+        expect(db.doc('foo/bar').firestore).to.be.an.instanceof(Firestore);


Is there a way we could define equality of Firestore instances based on e.g. the project, database, and app name?

I don't mean necessarily adding equals methods or something like that, but just an assertion we could use in tests? That way we could assert the Firestore instances are equivalent and use the same assertion for both cases.

As it stands, verifying that the thing returned is an instance of Firestore ... isn't much of a test.

I changed the shim to pass the Firestore instance between types, which is likely what we need to to once we productize this code path. This change removes the need to special-case this here.

wilhuff · 2020-07-16T17:43:51Z

packages/firestore/test/integration/api/database.test.ts

+        if (usesModularApi()) {
+          // The modular API throws an exception rather than rejecting the
+          // Promise, which matches our overall handling of API call violations.
+          expect(() => firestore.clearPersistence()).to.throw(expectedError);


If this is the behavior we want, why not change the classic version to match?

I think this meets the bar for "breaking change" since it throws instead of returning a rejected Promise.

wilhuff · 2020-07-16T17:45:10Z

packages/firestore/test/integration/api/fields.test.ts

-        });
-    });
-  });
+  // timestampInSnapshots is not support in the modular API.


Off topic: are we removing this option at Fireconf?

I am planning to do so: go/firestore-breaking-changes

wilhuff · 2020-07-16T17:47:12Z

packages/firestore/test/integration/api/validation.test.ts

    });

-    validationIt(persistence, 'enforces minimum cache size', db => {
+    validationIt(persistence, 'enforces minimum cache size', () => {
+      const db = newTestFirestore('testProjectId');


This is an invalid project ID. Only lowercase, numbers, and dashes are allowed.

Uses "test-project" here and elsewhere (via a new TEST_PROJECT export).

wilhuff · 2020-07-16T17:53:08Z

packages/firestore/test/integration/api/validation.test.ts

-        'Function CollectionReference.doc() requires between 0 and ' +
-          '1 arguments, but was called with 2 arguments.'
-      );
+      if (usesModularApi()) {


Since the only differences in the error messages are the function name and argument position, it seems like we could redefine these tests as the common set and those that are additional for the classic API. As it stands there's nothing requiring any of the equivalent validation checks to be identical. Given their superficial dissimilarity it seems possible for someone to overlook that a message needs to be updated in both places.

I'm not actually sure this is a good idea, but I thought I'd mention the idea to see if it resonates with you at all.

Done. This should also go away as we productionize the shim.

wilhuff · 2020-07-16T17:56:17Z

packages/firestore/test/integration/api/fields.test.ts

+const FieldPath = firebaseExport.FieldPath;
+const FieldValue = firebaseExport.FieldValue;
+const Timestamp = firebaseExport.Timestamp;
+const usesModularApi = firebaseExport.usesFunctionalApi;


Could we make this consistent throughout?

Oops. Done.

wilhuff

LGTM

schmidt-sebastian force-pushed the mrschmidt/sharedtests branch 4 times, most recently from 688c6e1 to e162085 Compare July 9, 2020 04:18

schmidt-sebastian force-pushed the mrschmidt/fixclear branch 2 times, most recently from ca0261e to ddddda8 Compare July 9, 2020 05:19

schmidt-sebastian force-pushed the mrschmidt/sharedtests branch 2 times, most recently from 8dbe197 to 4cc627a Compare July 9, 2020 17:05

schmidt-sebastian requested review from Feiyang1, hiranya911 and hsubox76 as code owners July 9, 2020 17:05

Base automatically changed from mrschmidt/fixclear to master July 9, 2020 17:43

schmidt-sebastian force-pushed the mrschmidt/sharedtests branch from 4cc627a to 3f79ac7 Compare July 9, 2020 17:47

Shared Integration Tests

bcf91c6

schmidt-sebastian force-pushed the mrschmidt/sharedtests branch from 3f79ac7 to bcf91c6 Compare July 9, 2020 17:52

Create smooth-shirts-teach.md

6ab963b

schmidt-sebastian assigned wilhuff Jul 9, 2020

schmidt-sebastian added the api: firestore label Jul 9, 2020

schmidt-sebastian requested a review from wilhuff July 9, 2020 18:20

wilhuff reviewed Jul 10, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Jul 10, 2020

schmidt-sebastian commented Jul 10, 2020

View reviewed changes

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Jul 10, 2020

schmidt-sebastian added 2 commits July 10, 2020 14:31

Share test files

8519f4a

Rename useModularApi

aac1f96

schmidt-sebastian added 2 commits July 10, 2020 14:56

Merge branch 'master' into mrschmidt/sharedtests

1b67c40

Fix firebase_export

c89064f

wilhuff reviewed Jul 16, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Jul 16, 2020

schmidt-sebastian added 2 commits July 16, 2020 13:54

Feedback

5fa9d3f

Formatting

b9ef49b

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Jul 16, 2020

wilhuff approved these changes Jul 16, 2020

View reviewed changes

schmidt-sebastian added 3 commits July 16, 2020 14:39

Undo TEST_PROJECT import since it breaks integration tests

869745c

Test update

0e25b67

...

76a839a

schmidt-sebastian force-pushed the mrschmidt/sharedtests branch from 6df3719 to 76a839a Compare July 16, 2020 23:21

schmidt-sebastian added 6 commits July 16, 2020 16:47

Lint

5ed0490

Next attempt

287f365

Use lerna

f94916d

Query fix

bba69f1

Merge branch 'master' into mrschmidt/sharedtests

fc11280

Test after master merge

b6abc27

schmidt-sebastian merged commit ea699fa into master Jul 17, 2020

schmidt-sebastian deleted the mrschmidt/sharedtests branch July 17, 2020 06:27

firebase locked and limited conversation to collaborators Aug 17, 2020

Share Integration Tests between firestore-exp and the main SDK #3374

Share Integration Tests between firestore-exp and the main SDK #3374

Uh oh!

Conversation

schmidt-sebastian commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset is good to go

Uh oh!

google-oss-bot commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

@firebase/firestore/exp

@firebase/firestore/lite

Test Logs

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

wilhuff commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmidt-sebastian commented Jul 10, 2020

Uh oh!

schmidt-sebastian commented Jul 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian commented Jul 8, 2020 •

edited

Loading

changeset-bot bot commented Jul 8, 2020 •

edited

Loading

google-oss-bot commented Jul 8, 2020 •

edited

Loading

`@firebase/firestore/exp`

`@firebase/firestore/lite`

wilhuff commented Jul 10, 2020 •

edited

Loading