Skip to content

removes extra broken call of callHttpsTrigger #431

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.

Sign up for GitHub

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 1 commit into from
Apr 19, 2019
Merged

removes extra broken call of callHttpsTrigger #431

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions integration_test/functions/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ export const integrationTests: any = functions
.collection('tests')
.doc(testId)
.set({ test: testId }),
// Invoke a callable HTTPS trigger.
callHttpsTrigger('callableTests', { foo: 'bar', testId }, baseUrl),
// A Remote Config update to trigger the Remote Config tests.
// admin.credential
Expand All @@ -150,8 +151,6 @@ export const integrationTests: any = functions
.storage()
.bucket()
.upload('/tmp/' + testId + '.txt'),
// Invoke a callable HTTPS trigger.
callHttpsTrigger('callableTests', { foo: 'bar', testId }),
Copy link
Contributor

@thechenky thechenky Apr 19, 2019

Choose a reason for hiding this comment

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

Was this call just not needed? Trying to understand why it was removed and not modified to include three fields.

Copy link
Contributor Author

@joehan joehan Apr 19, 2019

Choose a reason for hiding this comment

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

We only need to invoke this once, which we do earlier in the file. I think this was the result of bad merge conflict resolution by git.

It looks like when the merge between public and private happened, this line was moved in one branch and modified to the new signature in the other, so git left both changes in the merge.

Copy link
Contributor

@thechenky thechenky Apr 19, 2019

Choose a reason for hiding this comment

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

Hm...that sounds odd that this happened. Also we should always run the integration tests prior to releasing to avoid this in the future.

Copy link
Contributor Author

@joehan joehan Apr 19, 2019

Choose a reason for hiding this comment

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

Yeah, this one is on me - i ran them before the merge and must have forgotten to rerun afterwards

// Invoke the schedule for our scheduled function to fire
callScheduleTrigger('schedule', 'us-central1'),
])
Expand Down