test: Add test coverage for logical termination by cherylEnkidu · Pull Request #1743 · googleapis/nodejs-firestore

cherylEnkidu · 2022-06-30T20:48:45Z

No description provided.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

MarkDuckworth

The new tests look fine, but the use of promises could be improved for readability and consistency. I gave a few suggestions for improvement.

MarkDuckworth · 2023-12-11T17:14:03Z

+
+  it('can stream()', done => {
+    let received = 0;
+    return randomCol


You shouldn't need to return the result here. Returning anything could be misleading, since it could be confused with returning a promise, and I think this code is returning a Stream.

You use the done callback to signal that the test is complete instead of returning a promise. That's fine since that pattern is used in other tests in this file.

MarkDuckworth · 2023-12-11T17:20:22Z


+  it('has limit() method on stream()', done => {
+    let received = 0;
+    Promise.all([addDocs({foo: 'a'}, {foo: 'b'})]).then(() => {


Wrapping this statement in Promise.all(...) is misleading, and I don't think it adds value. Consider instead

addDocs(...).then(...

MarkDuckworth · 2023-12-11T17:22:28Z

+
+  it('can run limit(num), where num is larger than the collection size on stream()', done => {
+    let received = 0;
+    Promise.all([addDocs({foo: 'a'}, {foo: 'b'})]).then(() => {


MarkDuckworth · 2023-12-11T17:24:34Z

  });

-  it('has offset() method', async () => {
+  it('has offset() method on get()', async () => {


Offset method is on Query. Maybe consider rewording this test to 'can use offset() method with get()'

MarkDuckworth · 2023-12-11T17:24:43Z

    expectDocs(res, {foo: 'b'});
  });

+  it('has offset() method on stream()', done => {


Offset method is on Query. Maybe consider rewording this test to 'can use offset() method with get()'

MarkDuckworth · 2023-12-11T17:25:05Z


+  it('has offset() method on stream()', done => {
+    let received = 0;
+    Promise.all([addDocs({foo: 'a'}, {foo: 'b'})]).then(() => {


Same concern with use of Promise.all

MarkDuckworth · 2023-12-11T17:25:10Z

+
+  it('can run offset(num), where num is larger than the collection size on stream()', done => {
+    let received = 0;
+    Promise.all([addDocs({foo: 'a'}, {foo: 'b'})]).then(() => {


Same concern with use of Promise.all

MarkDuckworth

Approve with additional recommendations on clean up.

MarkDuckworth · 2023-12-14T16:42:27Z

+    for await (const doc of stream) {
+      expect(doc).to.be.an.instanceOf(QueryDocumentSnapshot);
+      ++received;
+    }


Nice use of for await...of!

MarkDuckworth · 2023-12-14T16:48:07Z

+    await addDocs({foo: 'a'}, {foo: 'b'}).then(async () => {
+      const stream = randomCol.orderBy('foo').limit(1).stream();
+      for await (const doc of stream) {
+        expect(doc).to.be.an.instanceOf(QueryDocumentSnapshot);
+        ++received;
+      }
+
+      expect(received).to.equal(1);
+    });


Nit: since you have an async test, you can drop the use of promise.then(...) and make the code even more readable.

await addDocs({foo: 'a'}, {foo: 'b'}); const stream = randomCol.orderBy('foo').limit(1).stream(); for await (const doc of stream) { expect(doc).to.be.an.instanceOf(QueryDocumentSnapshot); ++received; } expect(received).to.equal(1);

MarkDuckworth · 2023-12-14T16:48:24Z

+    await addDocs({foo: 'a'}, {foo: 'b'}).then(async () => {
+      const stream = randomCol.orderBy('foo').limit(3).stream();
+      for await (const doc of stream) {
+        expect(doc).to.be.an.instanceOf(QueryDocumentSnapshot);
+        ++received;
+      }
+      expect(received).to.equal(2);
+    });


Same nit here.

MarkDuckworth · 2023-12-14T16:48:48Z

+    await addDocs({foo: 'a'}, {foo: 'b'}).then(async () => {
+      const stream = randomCol.orderBy('foo').offset(1).stream();
+      for await (const doc of stream) {
+        expect(doc).to.be.an.instanceOf(QueryDocumentSnapshot);
+        ++received;
+      }
+
+      expect(received).to.equal(1);
+    });


same nit here

product-auto-label Bot added size: m Pull request size is medium. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Jun 30, 2022

cherylEnkidu force-pushed the cheryllin/RunQueryTest branch 2 times, most recently from f91e1a5 to bb2015d Compare July 4, 2022 14:32

product-auto-label Bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jul 4, 2022

cherylEnkidu force-pushed the cheryllin/RunQueryTest branch from bb2015d to da544d4 Compare July 4, 2022 14:51

product-auto-label Bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Jul 4, 2022

cherylEnkidu force-pushed the cheryllin/RunQueryTest branch from da544d4 to 5bf5e0b Compare July 4, 2022 15:08

MarkDuckworth approved these changes Jul 27, 2022

View reviewed changes

cherylEnkidu force-pushed the cheryllin/RunQueryTest branch from 5bf5e0b to 885ecec Compare December 7, 2023 20:55

product-auto-label Bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Dec 7, 2023

cherylEnkidu marked this pull request as ready for review December 7, 2023 21:44

cherylEnkidu requested review from a team December 7, 2023 21:44

cherylEnkidu force-pushed the cheryllin/RunQueryTest branch from 2897101 to 2d168b0 Compare December 7, 2023 21:50

product-auto-label Bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Dec 7, 2023

ehsannas reviewed Dec 7, 2023

View reviewed changes

Comment thread dev/system-test/firestore.ts Outdated

cherylEnkidu added 3 commits December 8, 2023 11:33

test: Add test coverage for logical termination

8905413

improve tests

8343671

remove .only

bd095b1

cherylEnkidu force-pushed the cheryllin/RunQueryTest branch from c3d8246 to bd095b1 Compare December 8, 2023 16:33

🦉 Updates from OwlBot post-processor

94d270f

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

cherylEnkidu added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 8, 2023

yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 8, 2023

cherylEnkidu assigned MarkDuckworth Dec 8, 2023

MarkDuckworth reviewed Dec 11, 2023

View reviewed changes

cherylEnkidu force-pushed the cheryllin/RunQueryTest branch from 7b1d052 to 6307c44 Compare December 13, 2023 15:48

Address feedback

c02f2c5

cherylEnkidu force-pushed the cheryllin/RunQueryTest branch from 8c1ae2f to c02f2c5 Compare December 13, 2023 15:53

Merge branch 'main' into cheryllin/RunQueryTest

fe6d867

MarkDuckworth approved these changes Dec 14, 2023

View reviewed changes

Address commends 2

d80022c

cherylEnkidu merged commit 85f1008 into main Dec 14, 2023

cherylEnkidu deleted the cheryllin/RunQueryTest branch December 14, 2023 20:52

Conversation

cherylEnkidu commented Jun 30, 2022

Uh oh!

Uh oh!

MarkDuckworth left a comment

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!

Uh oh!

MarkDuckworth left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants