-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Add new "quickstart" samples #547
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
|
It's sad to see that there are no tests for these, especially considering these will be the first samples users try out. |
I'm willing to write tests, but I need a crash course on testing python-docs-samples. |
I can add them, they're just inherently hard to test. As soon as you import these modules they do stuff (side-effects), this is why all of samples are in functions - so that tests can setup fixtures to mock whatever is needed or setup environment variables before calling the function. You've also hard-coded the project id, which means I have to do at least some mocking. It's also inconsistent with all of the parameters shown in other samples (which are just function args). |
|
I recognize that they're harder to test (the tests for Node.js versions of these samples were a pain). Perhaps @omaray can concede some simplicity for more maintainable/testable samples? i.e. wrap in a function? Hard-coding the project ID is a "product requirement" for these samples (these "quickstart" style samples will be the only ones that do so, all the rest can infer from the environment). Also, I tried TESTING.md, but |
|
@jonparrott happy to chat in person about this. |
|
We were hoping this week if possible. But we can also check-in something for now that works (that we test manually to confirm) and then add the test next week. That wouldn't be a concern to me. By the way, @jmdobry is using the "gcloud" module in his examples. Can we now switch to using the google-cloud-{api} one instead? So |
With python, can the test do the setup stuff before import the sample? (That's essentially what we do in Node.js.) Are imports required to be the first statements in a file? |
We can do that, it's just pretty hacky.
That would be fine. |
|
@jmdobry if you rebase on master, you can use google-cloud (and tests should pass) |
8bc413d to
fef2506
Compare
|
Done |
fef2506 to
470e41b
Compare
|
I removed the explicit project IDs. |
|
I wrapped each snippet in a method definition, does that make it easier for you to test? I also added snippets for Language and Vision. Speech is the only one missing ( |
|
Also, I ran all of the samples manually to make sure they work. Except for Speech, this PR now has "quickstart" sample parity with GoogleCloudPlatform/ruby-docs-samples#77 and the corresponding Node.js samples (which are already on |
These are intended to be simple examples that explicitly show:
Products covered:
For reference, see the corresponding samples in other languages:
masterin variousquickstart.jsfiles)Thanks!