-
Notifications
You must be signed in to change notification settings - Fork 58
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
Conversation
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
spec/v2.spec.ts
Outdated
@@ -460,6 +463,265 @@ describe('v2', () => { | |||
}); | |||
}); | |||
|
|||
describe('firestore', () => { | |||
describe('should resolve document path', () => { |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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", |
There was a problem hiding this comment.
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
2nd gen firestore triggers support is coming soon! This PR will add support for mocking 2nd gen firestore triggers.
Example: