Skip to content

feat: Support Logical Termination on RunQueryResponse#1741

Merged
cherylEnkidu merged 3 commits intomainfrom
RunQuery
Jun 30, 2022
Merged

feat: Support Logical Termination on RunQueryResponse#1741
cherylEnkidu merged 3 commits intomainfrom
RunQuery

Conversation

@cherylEnkidu
Copy link
Contributor

Logical Termination: RunQueryResponse will emit "end" event when the "data" event proto which has "done" field set to true.

@cherylEnkidu cherylEnkidu requested review from a team as code owners June 23, 2022 21:52
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Jun 23, 2022
})
.on('end', () => {
if (hasResolve) return;
hasResolve = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

hasResolved

Also, did you check whether you can just invoke resolve() again?

})
.on('end', () => {
expect(received).to.equal(2);
expect(++counter).to.equal(1);
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian Jun 23, 2022

Choose a reason for hiding this comment

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

You should test that you only receive a single 'end' callback. One think you can do is change this to:

let endCounter = 0;
query
        .stream()
        .on('data', doc => {
          expect(doc).to.be.an.instanceOf(DocumentSnapshot);
          ++receivedCounter;
        })
        .on('end', () => {
          expect(received).to.equal(2);
         ++endCounter;
          setImmediate(() => {
             expect(endCounter).to.equal(1);
             callback();
          });
        });
    });
  });

This way, you ensure that once all test processing is done, there is an additional check that verifies that 'end' is only called once. Your test right now does the same thing, but once you call callback test is considered done and it won't fail even if any expectations fail after.

@cherylEnkidu cherylEnkidu force-pushed the RunQuery branch 2 times, most recently from 5b04be1 to cd595cf Compare June 28, 2022 00:48
@cherylEnkidu cherylEnkidu requested a review from wu-hui June 28, 2022 15:28
@cherylEnkidu cherylEnkidu merged commit 07de28a into main Jun 30, 2022
@cherylEnkidu cherylEnkidu deleted the RunQuery branch June 30, 2022 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the googleapis/nodejs-firestore API. size: xl Pull request size is extra large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants