-
Notifications
You must be signed in to change notification settings - Fork 212
Add integration test for firebase-functions binary #1028
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
… into dl-integration-tests
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.
Nice stuff! yay tests!
package: ./package.json | ||
reporter: spec | ||
require: | ||
- 'ts-node/register' | ||
spec: spec/**/*.spec.ts |
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.
removing this just means that we now have test files outside the spec/
dir and they don't need spec in the title, correct?
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.
Yes - having the spec defined here made it impossible for me to not run them for integration tests. So we now pass in the path/glob of test we want to run in the npm script definition (in package.json) itself.
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.
Ahhh the changed line - "test": "mocha --file ./mocha/setup.ts spec/**/*.spec.ts ",
. Thanks!
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.
Hmm... there's no other way to achieve our goal? Changing mocha to need a --file means that running an individual file's tests with npx mocha
got harder.
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.
Hmm don't you still need to pass individual file via npx mocha
, e.g. npx mocha spec/path/to/my/test.ts
?
I'm not sure what devx I just broke here. I don't know if other ways exists, but happy to explore.
package: ./package.json | ||
reporter: spec | ||
require: | ||
- 'ts-node/register' | ||
spec: spec/**/*.spec.ts |
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.
Hmm... there's no other way to achieve our goal? Changing mocha to need a --file means that running an individual file's tests with npx mocha
got harder.
) { | ||
const sleep = () => { | ||
return new Promise<void>((resolve) => { | ||
setTimeout(() => resolve(), sleepMs); |
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: you can just pass a function rather than a lambda that calls the function:
setTimeout(resolve, sleepMs)
} | ||
}).timeout(TIMEOUT_L); | ||
|
||
if (semver.gt(process.versions.node, '13.2.0')) { |
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: since 13 isnt' a supported major it would have been fine to say '14'
Add integration test for firebase-functions binary.
The integration setup is more robust than what we had before - it builds and installs the Functions SDK package at the branch commit and try to run the produced binary that ships with the Functions SDK.
We also modify the github CI/release script setting to run the integration test as part of its workflow.