Skip to content
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

Fix functions deploy integration test script #1703

Merged
merged 6 commits into from
Oct 8, 2019
Merged

Conversation

laurenzlong
Copy link
Contributor

Description

Several fixes to the integration test:

  • Properly set access token
  • Update outdated syntax inside the test functions
  • Remove dependency on Sinon, stubbing and restoring was flakey.
  • Leave functions undeleted if test failed (for debugging via functions logs)

Scenarios Tested

node scripts/test-functions-deploy.js. Result below.

image

@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label Oct 8, 2019
@coveralls
Copy link

coveralls commented Oct 8, 2019

Coverage Status

Coverage remained the same at 65.054% when pulling cbb8130 on ll-fix-cf3-tests into 1829c39 on master.

Copy link
Contributor

@kevinajian kevinajian left a comment

Choose a reason for hiding this comment

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

Thanks for the fix

exports.dbAction = functions.database.ref("/input/{uuid}").onWrite(function(event) {
console.log("Received event:", event);
return event.data.ref.root.child("output/" + event.params.uuid).set(event.data.val());
exports.dbAction = functions.database.ref("/input/{uuid}").onCreate(function(snap, context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious about why the switch from onWrite to onCreate for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the function receives a Snapshot, instead of a Change object. (We didn't need the "before" and "after" that Change provides.)

"firebase": "^3.5.0",
"firebase-admin": "latest",
"firebase-functions": "latest"
"firebase": "^3.5.0"
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 package.json file actually required? If the script is run node scripts/test-functions-deploy.js, and firebase is already a dev dependency of the package, then do we need to keep this file around?

@@ -25,9 +25,6 @@ var functionsConfig = require("../lib/functionsConfig");

var clc = require("cli-color");
var firebase = require("firebase");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fairly certain that firebase in the root package.json is really, really, really, really old - it would be great if it was updated. (This file is the only requirement for having firebase as a dependency - I've looked - so since you're updating it...)

@laurenzlong laurenzlong merged commit 64c0613 into master Oct 8, 2019
@laurenzlong laurenzlong deleted the ll-fix-cf3-tests branch October 8, 2019 23:00
Elgarni added a commit to Elgarni/firebase-tools that referenced this pull request Oct 9, 2019
* master:
  change out tslint for eslint, new publish config (firebase#486)
  Remove scripts/package.json and update firebase version. (firebase#1704)
  Fix functions deploy integration test script (firebase#1703)
  Loop the modules properties without prototype methods. Fixes firebase#1687 (firebase#1694)
  Allow customers to configure the db setting 'strictTriggerValidation' (firebase#1702)
  [firebase-release] Removed change log and reset repo after 7.5.0 release
  7.5.0
  Fix export users (firebase#1690)
  Fix port issues in WSL (firebase#1699)
  Unremove but deprecate separate port for WebChannel. (firebase#1698)
  Release Firestore Emulator 1.9.0 and remove WebChannel workaround. (firebase#1689)
  update handlebars dependency (firebase#1686)
  [firebase-release] Removed change log and reset repo after 7.4.0 release
  7.4.0
  remove items from previews list (firebase#488)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants