Skip to content
This repository was archived by the owner on Mar 4, 2026. It is now read-only.

fix: Force http in the GAX module when using the GAX fallback and connecting to the emulator#1788

Merged
MarkDuckworth merged 4 commits intomainfrom
markduckworth/preferrest-emulator-support
Oct 17, 2022
Merged

fix: Force http in the GAX module when using the GAX fallback and connecting to the emulator#1788
MarkDuckworth merged 4 commits intomainfrom
markduckworth/preferrest-emulator-support

Conversation

@MarkDuckworth
Copy link
Copy Markdown
Contributor

fix: Force http in the GAX module when using the GAX fallback and connecting to the emulator

@MarkDuckworth MarkDuckworth requested review from a team October 14, 2022 20:48
@conventional-commit-lint-gcf
Copy link
Copy Markdown

conventional-commit-lint-gcf Bot commented Oct 14, 2022

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-label product-auto-label Bot added size: s Pull request size is small. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Oct 14, 2022
@dconeybe
Copy link
Copy Markdown
Contributor

Can you think about how you might write a unit or integration test for this? My first, probably dumb, idea is to spin up a local http server and try to connect to it and make sure the connection is done via http (and not https). Just something to think about, if you can find a practical way to test this. Maybe there is somewhere else you can "hook in" to make sure the correct protocol is used.

Comment thread dev/src/index.ts Outdated
Comment thread dev/src/index.ts Outdated
Copy link
Copy Markdown
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

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

LGTM but would prefer having a well defined type if possible.

@MarkDuckworth
Copy link
Copy Markdown
Contributor Author

Can you think about how you might write a unit or integration test for this? My first, probably dumb, idea is to spin up a local http server and try to connect to it and make sure the connection is done via http (and not https). Just something to think about, if you can find a practical way to test this. Maybe there is somewhere else you can "hook in" to make sure the correct protocol is used.

@dconeybe I think the best solution will be to configure all integration tests to run against the emulator. I'll touch base with the team on this and align with strategy for running against emulator vs production database.

Comment thread dev/src/index.ts Outdated
Comment thread dev/src/index.ts Outdated
Comment thread dev/src/index.ts Outdated
@dconeybe
Copy link
Copy Markdown
Contributor

Can you think about how you might write a unit or integration test for this? My first, probably dumb, idea is to spin up a local http server and try to connect to it and make sure the connection is done via http (and not https). Just something to think about, if you can find a practical way to test this. Maybe there is somewhere else you can "hook in" to make sure the correct protocol is used.

@dconeybe I think the best solution will be to configure all integration tests to run against the emulator. I'll touch base with the team on this and align with strategy for running against emulator vs production database.

But then the scenario where the emulator is not used is left untested. I definitely wouldn't block merging this PR on switching the tests to run against the Firestore emulator. But is there some way that you can write unit tests for the settings that get passed into new module.exports.v1(settings, gax)? It looks like it may be somewhat challenging, but it would be good to add such tests to avoid backsliding. Maybe just try it out to see if it's easy and timebox the work. If it ends up being unwieldy, then don't worry about it.

@MarkDuckworth MarkDuckworth merged commit 50747ad into main Oct 17, 2022
@MarkDuckworth MarkDuckworth deleted the markduckworth/preferrest-emulator-support branch October 17, 2022 19:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: firestore Issues related to the googleapis/nodejs-firestore API. size: s Pull request size is small.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants