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: Force http in the GAX module when using the GAX fallback and connecting to the emulator #1788

Merged
merged 4 commits into from Oct 17, 2022

Conversation

MarkDuckworth
Copy link
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 as code owners October 14, 2022 20:48
@conventional-commit-lint-gcf
Copy link

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
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.

dev/src/index.ts Outdated Show resolved Hide resolved
dev/src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@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
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.

dev/src/index.ts Outdated Show resolved Hide resolved
dev/src/index.ts Outdated Show resolved Hide resolved
dev/src/index.ts Outdated Show resolved Hide resolved
@dconeybe
Copy link
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 join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet

3 participants