Missing ./system-tests/

[beccasaurus]

There are no system tests!

IIUC these are authored in the gapic YAML file?

Needed urgently.

[beccasaurus]

/cc @lukesneeringer @jmdobry

[beccasaurus]

FYI I'm not sure how these system tests are authored/generated/run

All I know for sure is that nodejs-text-to-speech has no ./system-tests/ directory, but every other nodejs-* repo does 😄

[beccasaurus]

/cc @dizcology (re: lib generation)

[alexander-fenster]

Not every - some still don't (see https://github.com/googleapis/nodejs-os-login/tree/master/system-test). Generator can generate a very basic smoke test if defined in config yaml, but it's not the case for text-to-speech. Good system tests must be written manually, and in fact I'm not sure who is responsible for that. @lukesneeringer, who should write system tests for the autogen'd libraries - is it me or someone else, or is auto-gen'd smoke test enough?

[alexander-fenster]

@remi For fully autogen'd libraries like this one smoke test should be generated automatically as well, if GAPIC configuration file allows it (see example here: https://github.com/googleapis/googleapis/blob/6cb47db389b1670310a8d323658eb3d7148f7da1/google/cloud/vision/v1p1beta1/vision_gapic.yaml#L26) In this particular case there is no smoke test defined, hence no smoke test is generated. Even if the test is an auto-generated smoke test, we often place it into system-tests directory to make the directory layout similar across libraries.

Given that we have system tests for samples that verify that the API and client library work, I don't see a big problem in releasing this library with no auto-generated smoke test for now, especially if it's alpha.

Adding smoke tests in GAPIC config should be a responsibility of API owners, I'll be happy to regen the code after they are added.

[beccasaurus]

I'll keep this bug but unblock the merge & launch

$0.02 We really should do this and I would personally vote to block all new client libraries from going out until they have smoke tests, even through you're right about the samples, it seems irresponsible not to add a test via the YAML 🤷🏼

[beccasaurus]

Thanks for tracking down info about these

[jkwlui]

@beccasaurus who should I contact in the product team to get some system-tests in the protos?

[beccasaurus]

I filed a Feature Request to add the smoke test configuration

@alexander-fenster @kinwa91 Do y'all this is important enough to warrant a bug & GitHub issue

I'd say that... for APIs which have no samples+system tests alongside the client library, which are part of the client library's CI tests... Smoke Tests are important. But for client libraries where integration samples are inside the client library repository and run alongside the general client library test suite, they are not particularly important.

If samples were embedded alongside client libraries in all languages, I'd say the smoke tests aren't very critical

As that's not the case today... they're important IMO!

[JustinBeckwith]

Good news - the smoke test landed!
0bf9140#diff-af5d09d72c5ecdc41020800a346fa78c

I think we're good here :)