Skip to content

Add testing support for 2nd gen firestore triggers #200

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 8 commits into from
Apr 15, 2023

Conversation

blidd-google
Copy link
Contributor

2nd gen firestore triggers support is coming soon! This PR will add support for mocking 2nd gen firestore triggers.

Example:

// index.ts
import { onDocumentCreated } from "firebase-functions/v2/firestore";

export const fn = onDocumentCreated("foo/{bar}", (event) => {...});

// index.test.ts
import * as test from "firebase-functions-test";
import { fn } from './index';

const {wrap} = test();
const wrappedFn = wrap(fn);
wrappedFn({
  data: {
    a: 'b',
    c: 'd',
  },
  params: {
    bar: 123,
  },
});

@blidd-google blidd-google self-assigned this Mar 21, 2023
package.json Outdated
@@ -47,7 +47,7 @@
"@types/mocha": "^5.2.7",
"chai": "^4.2.0",
"firebase-admin": "^10.1.0",
"firebase-functions": "^4.0.0-rc.0",
"firebase-functions": "file:../firebase-functions",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woah that was the fastest PR review of all time!! will do

Choose a reason for hiding this comment

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

you should be good to grab v 4.3.0 now

@blidd-google blidd-google requested a review from colerogers March 21, 2023 18:09
spec/v2.spec.ts Outdated
@@ -460,6 +463,265 @@ describe('v2', () => {
});
});

describe('firestore', () => {
describe('should resolve document path', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove the "should resolve" ("document path")

// Created or Deleted event for the onWritten trigger), then we
// include the user's before/after object in the mock event and
// use an example snapshot for the other.
if (data instanceof Object && (data.before || data.after)) {
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 necessary? (if so can you document why we are checking its an object)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reused the logic from getOrCreateDataSnapshotChange, but actually the data instanceof Object conditional is probably not necessary.

// Created or Deleted event for the onWritten trigger), then we
// include the user's before/after object in the mock event and
// use an example snapshot for the other.
if (data instanceof Object && (data.before || data.after)) {
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 || suppose to be a && ? Does getOrCreateDocumentSnapship still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used || instead of && because I thought it may be surprising for users who specify an object like

const partial = {
  data: {
    before: {
      foo: 'bar',
    },
}

and expect the generated mock event data to include event.data.before, but if we use &&, they will be required to specify both the before and after objects, and if they don't, the data they provide will be overwritten and they will see the example data in the cloud event instead.

Copy link

@colerogers colerogers left a comment

Choose a reason for hiding this comment

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

Looks good - just update the package.json & get tyler's signoff

package.json Outdated
@@ -57,7 +57,7 @@
},
"peerDependencies": {
"firebase-admin": "^8.0.0 || ^9.0.0 || ^10.0.0 || ^11.0.0",
"firebase-functions": "^4.0.0",
"firebase-functions": "file:../firebase-functions",

Choose a reason for hiding this comment

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

same here - 4.3.0

@blidd-google blidd-google merged commit 49bb43e into master Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants