Initial implementation of GAPIC generator acceptance harness #222

vchudnov-g · 2019-10-14T23:54:19Z

This PR includes most but not all of the needed infrastructure for the acceptance harness, here called "qualifier". It is invoked by gapic-showcase qualify LANG (verified with gapic-showcase qualify python). We will use it to verify generator behavior (particularly in terms of the generated samples) against a running showcase server

Features included in this PR:

this adds a new showcase command qualify, which runs the showcase server in-process in preparation for testing samples against this running server
acceptance checks ("scenarios") can be added directly as sub-directories of cmd/gapic-showcase/qualifier/acceptance_suite/
at run time, the harness copies scenario configs to a clean sandbox where generation/testing takes place
common protos can be included in any place in the created sandbox by means of "include" files in the desired place within the scenario directory structure
this includes a basic acceptance suite "basic-check" which exercises a bunch of the functionality, most notably the use of include files (**/include.XXX) to include specific files and whole directories at various levels of the scenario hierarchy
this allows passing --options to pass additional options to the generator (for example, this is needed for Go but not for Python)
the scenario configs are packaged within the released binary (using the Packr package)

To be done in subsequent PRs:

add tests for some of the functionality here
run sample-tester for configured tests of the generated samples running against a live showcase server (right now there is no sample test support)
process a specification indicating expected generation failures to make sure we generate errors when we should (right now we expect generation to always succeed)
allow generating directly from gapic-generator as well (right now we invoke the generator as a protoc plugin)
eventually, when the testing service is implemented in showcase: we will also allow specifying expectations for RPCs received the server

To be done in an on-going basis:

add more scenarios to capture the various checks GAPIC generators must pass: samplegen features, samplegen error cases, libgen error cases

The debug logger will be switched on by CLI flags later; right now it's hard-coded.

Instead of fetching a relese. This is for the development prepare-to-qualify script.

Capture the language name and options in preparation for running the protoc plugin from this acceptance harness

A Suite is a collection of Scenarios

Also fix a bug where we were not crrectly retrieving files from packr.Box

Need to store files as relative directories

The paths are relative to the scenario dir (in either the source file box or in the sandbox)

protoc still not working

In the future, we'll be able to configure expected failures via a config file, and check for those as well. This also includes miscellaneous documentation improvements.

In the process, constructed a sample config for the echo service and made the hard-coded generator default be protoc-gen-python_gapic via protoc.

This creates a package `qualifier`

This involved refactoring the "run" command so that we can start a server and not block on it ending.

We now officially bundle the acceptance files within the Go binary.

Otherwise we'll trivially succeed on zero checks.

codecov · 2019-10-14T23:56:58Z

Codecov Report

Merging #222 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #222   +/-   ##
=======================================
  Coverage   99.61%   99.61%           
=======================================
  Files          13       13           
  Lines        1036     1036           
=======================================
  Hits         1032     1032           
  Misses          2        2           
  Partials        2        2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5acf67...fe47602. Read the comment docs.

vchudnov-g · 2019-10-15T17:51:32Z

cmd/gapic-showcase/qualify.go

+		ShowcasePort: 7469,
+	}
+	qualifyCmd := &cobra.Command{
+		Use:   "qualify",


I'm actually reconsidering this command name now. I'm concerned it's not completely obvious what it does. Maybe "check-generator" would be clearer. WDYT? Other ideas?

If we decide to change it, I can change the package name to match.

check-generator seems just as vague considering we have several different generators :)

gapic-showcase acceptance might be another option.

qualify is pretty good imo, but the usage documentation for the command should be solid & clarify.

check-generator seems just as vague considering we have several different generators :)

But the usage would have a direct object, like this:

gapic-showcase check-generator python

so it would be clear which generator is being tested.

I don't like acceptance because it's not a verb.

And good point: I did not have a long usage message. Added.

But the usage would have a direct object

Ah, yes ok then that is reasonable. "check" to me isn't a strong/descriptive enough word. <rhetorical> What are we checking? That it exists? That it compiles? </rhetorical> I think "qualify-generator" is a better sounding command because it implies that some sort of validation and comparison of the generator against expected results happens.

Now, digging down into the the "direct object" you mention: is that a sub-command or just an argument? Is it a flag with accepted values (i.e. python, java, go, etc.)? Or free form?

Now, digging down into the the "direct object" you mention

nvm I read the LongUsage, it's an exact arg. Thanks for adding that.

vchudnov-g · 2019-10-16T16:51:10Z

cmd/gapic-showcase/qualifier/acceptance_suite/basic-check/echo.proto

+service Echo {
+  // This service is meant to only run locally on the port 7469 (keypad digits
+  // for "show").
+  option (google.api.default_host) = "localhost:7469";


I'm thinking this file should become a symlink to the canonical echo.proto file, rather than a copy.

+1 I was going to say the exact same thing!

Unfortunately, Packr does not include symlinked files....

I suggest for this PR, we leave this as is: an explicitly copied files. This makes it very explicit what's being tested. Packr does compress the files being included, so additional instances of the file should have just a trivial cost.

If we do become concerned about repeated files as we add to the acceptance suite (for example, if the repeated files get decompressed all at once rather than on-demand), we could look into being able to include showcase API protos via include files. But I suggest we defer that for now.

I'm OK with doing this for now.

additional instances of the file should have just a trivial cost

In this case, I'm not worried as much about size costs but rather maintenance costs. If we add a new annotation or one of the existing annotations changes, or we add a new RPC, we will be updating N echo.proto files and it is too easy to miss one. It is easy to grow out of sync with multiple copies and then our understanding of what is actually being tested is shaky i.e. we see echo.proto and assume a lot that might actually no longer be true for an individual copy.

we could look into being able to include showcase API protos via include files

This should be one of the first things evaluated, especially as it might apply to api-common-protos && googleapis/googleapis. Checking in multiple copies of these sources will get out of hand.

To be clear, we already have this for anything under schema/, but not for the showcase files themselves.

You make a good argument for doing this for showcase files: not having to repeat the modification in all copies. If we don't do it, we'd find out pretty quickly which copies we missed updating because tests would fail---but that's a bad experience. If we do do it, it would make it a lot simpler to update the cases where we expect generation to succeed.

For cases where we expect generation to fail, depending on what we want the error to be, we may want to have copies rather than symlinks so that we can modify them to cause the error we want. For these cases, we may still run into the problem that if a newly required annotation is not present in the previously copied-and-modified file, the error message we get may be due to that instead of the cause we expected, and the test would thus fail. I don't think we can get around that other than by making sure to run the acceptance harness whenever a new annotation comes in, so we can update these cases manually if needed.

For cases where we expect generation to fail, depending on what we want the error to be, we may want to have copies rather than symlinks

That's reasonable. But do they have to be a full copy of a showcase proto (e.g. echo.proto)? Can it just be a minimally working proto with just the one erroneous piece and have it named "gen_failure_{reason}.proto"? If we have 5 copies of echo.proto with entirely the same content except maybe a typo in an RPC name or 1 missing message in order to trigger a failure mode, it will be too easy to assume the wrong thing imo. Does that make sense? I just want the purpose of each added proto/input file to be very clear.

we may still run into the problem that if a newly required annotation is not present in the previously copied-and-modified file, the error message we get may be due to that instead of the cause we expected

Yeah that's fair, it should be obvious when something is missing and a developer should expect to have to update everything in these scenarios.

noahdietz

A couple of things:

From previous code reviews I think you mentioned that you disagree with Go naming practices. Most of my comments are WRT naming. I explained the reasoning in my first few comments but eventually opted to just use s/var1/var2 to save time writing/reading the comments. The reasoning is generally the same. I'm trying to find a middle ground in suggestions, so please be patient with the numerous naming comments.
I would really prefer that not all of the qualifier code be in the cmd/gapic-showcase directory. Can we move most of the library-oriented stuff to a separate pkg? Ideally the cmd directory contains CLI/executable style code.
How big do we expect the acceptance_suite to get? Packing most of api-common-protos in the binary is already quite worrisome to me. This could become a lot of static files to pack into the binary...

cmd/gapic-showcase/qualifier/acceptance_suite/basic-check/foo/bar/include.wdREAD

cmd/gapic-showcase/qualifier/generator.go

noahdietz · 2019-10-16T16:55:30Z

cmd/gapic-showcase/qualifier/acceptance_suite/basic-check/echo.proto

+service Echo {
+  // This service is meant to only run locally on the port 7469 (keypad digits
+  // for "show").
+  option (google.api.default_host) = "localhost:7469";


+1 I was going to say the exact same thing!

cmd/gapic-showcase/qualifier/scenario.go

noahdietz · 2019-10-16T21:44:08Z

cmd/gapic-showcase/qualify.go

+		ShowcasePort: 7469,
+	}
+	qualifyCmd := &cobra.Command{
+		Use:   "qualify",


check-generator seems just as vague considering we have several different generators :)

gapic-showcase acceptance might be another option.

qualify is pretty good imo, but the usage documentation for the command should be solid & clarify.

noahdietz · 2019-10-16T21:53:12Z

cmd/gapic-showcase/run.go

+// Shutdown will immediately start a graceful shutdown of the servers.
+func (srv *ShowcaseServers) Shutdown() {
+	if srv.fb != nil {
+		srv.fb.Shutdown() // unfortunately, this always results in an en error log


I'm not sure what you mean here. There is an if statement that only logs when shutting down fails for some reason.

If when you run your code and it behaves as you describe please file a bug/open a PR to change it.

OK, I'll do that separately. Not sure whether that print was coming from showcase code or from the http server library.

noahdietz · 2019-10-16T21:54:27Z

go.mod

+	github.com/hashicorp/go-version v1.2.0 // indirect
+	github.com/karrick/godirwalk v1.12.0 // indirect
+	github.com/mitchellh/gox v1.0.1 // indirect
+	github.com/rogpeppe/go-internal v1.5.0 // indirect


please remove the indirect dependencies, these get annoying with RenovateBot and are not strictly necessary

OK, good to know.

And include a README about them

vchudnov-g

Thanks for the review, Noah! I responded to your comments.

naming: I still adhere to the maxim "make your identifier names just long enough to make it easy to read the code, but no longer". I liked a lunch of your suggestions and adopted them, was borderline on others but adopted them, and pushed back on just a few.
packaging

I would really prefer that not all of the qualifier code be in the cmd/gapic-showcase directory. Can we move most of the library-oriented stuff to a separate pkg? Ideally the cmd directory contains CLI/executable style code.

Most of the code is in the qualifier package already. Do you mean moving it elsewhere?

Size

How big do we expect the acceptance_suite to get? Packing most of api-common-protos in the binary is already quite worrisome to me. This could become a lot of static files to pack into the binary...

The number of checks could grow quite a bit, but they will almost always include just the same files: the showcase protos and the common protos. The nice thing is that Packr compresses the files, so these additional inclusions should not take up extra space in the binary. That said, I'm not sure whether the files get decompressed on the fly or all at once; the latter could be a problem. However, as I mention below, even in that case we can insert the files into the execution sandboxes via include files in the scenarios, so that these files only get packaged/included/decompressed in Packr only once.

In some of the error scenarios we'll check we may well include proto or config files with intentional errors, if we want to test those behaviors, so in those cases we'll have to include those files explicitly. But there will probably be a lot fewer of those, they will be a lot smaller (I'm guessing), and again, if they're repeated in multiple scenarios, we can extend the include mechanism to account for those.

cmd/gapic-showcase/qualifier/acceptance_suite/basic-check/foo/bar/include.wdREAD

cmd/gapic-showcase/qualifier/dependencies.go

vchudnov-g · 2019-10-19T03:00:23Z

cmd/gapic-showcase/qualify.go

+		ShowcasePort: 7469,
+	}
+	qualifyCmd := &cobra.Command{
+		Use:   "qualify",


And good point: I did not have a long usage message. Added.

vchudnov-g · 2019-10-19T03:05:43Z

go.mod

+	github.com/hashicorp/go-version v1.2.0 // indirect
+	github.com/karrick/godirwalk v1.12.0 // indirect
+	github.com/mitchellh/gox v1.0.1 // indirect
+	github.com/rogpeppe/go-internal v1.5.0 // indirect


OK, good to know.

vchudnov-g · 2019-10-21T19:20:12Z

cmd/gapic-showcase/qualifier/acceptance_suite/basic-check/echo.proto

+service Echo {
+  // This service is meant to only run locally on the port 7469 (keypad digits
+  // for "show").
+  option (google.api.default_host) = "localhost:7469";


Unfortunately, Packr does not include symlinked files....

I suggest for this PR, we leave this as is: an explicitly copied files. This makes it very explicit what's being tested. Packr does compress the files being included, so additional instances of the file should have just a trivial cost.

If we do become concerned about repeated files as we add to the acceptance suite (for example, if the repeated files get decompressed all at once rather than on-demand), we could look into being able to include showcase API protos via include files. But I suggest we defer that for now.

cmd/gapic-showcase/qualifier/acceptance_suite/basic-check/foo/bar/include.wdREAD

vchudnov-g · 2019-10-21T19:33:33Z

cmd/gapic-showcase/run.go

+// Shutdown will immediately start a graceful shutdown of the servers.
+func (srv *ShowcaseServers) Shutdown() {
+	if srv.fb != nil {
+		srv.fb.Shutdown() // unfortunately, this always results in an en error log


OK, I'll do that separately. Not sure whether that print was coming from showcase code or from the http server library.

noahdietz · 2019-10-22T00:01:02Z

naming: ... I liked a lunch of your suggestions and adopted them, was borderline on others but adopted them, and pushed back on just a few.

Thank you for meeting me in the middle.

packaging: Most of the code is in the qualifier package already. Do you mean moving it elsewhere?

Yeah, I mean moving the qualifier dir to something like /qualifier or internal/qualifier. Just so that all of the business logic is not in the CLI directory as a subdir. I could've been clearer in my original comment, thanks.

Size

I need to read your suggestions & think a bit more on this one.

vchudnov-g · 2019-10-22T22:21:50Z

On moving the qualifier/ package: I can start doing that if it won't screw up your code review, or we can defer that until we've resolved all the other comments. Which do you prefer?

noahdietz · 2019-10-22T22:27:04Z

defer that until we've resolved all the other comments

Let's defer, great idea. Don't want to lose all of the history!

vchudnov-g added 30 commits October 14, 2019 16:21

Fix typos

30cd85d

Bare-bones scaffold for acceptance harness

eaec552

Move most logging to a debug logger

7b3e100

The debug logger will be switched on by CLI flags later; right now it's hard-coded.

Iterate over all suite config dirs. Add a simple suite config.

9312158

Check needed dependencies. Add script to prepare them.

1ba8c2c

Start and stop gapic-showcase in the background.

f0abe48

Build gapic-showcase from local tree.

a65893d

Instead of fetching a relese. This is for the development prepare-to-qualify script.

Add some comments/TODos

4fac512

use go-trace package for debug statements

54ddeb5

Classify suite files by type

1139617

Add instructions for running protoc w Go plugin

8a339c3

Capture the language name and options in preparation for running the protoc plugin from this acceptance harness

Use packr to get files from disk

d09898f

Rename "Suite" to "Scenario"

c49b73d

A Suite is a collection of Scenarios

Add some TODOs

3baf0ac

Create sandbox directory with a per-run timestamp in its name

feec344

Also fix a bug where we were not crrectly retrieving files from packr.Box

WIP: Running protoc

683c0ff

Need to store files as relative directories

Make the file list stored with each scenario be relative paths

73b3768

The paths are relative to the scenario dir (in either the source file box or in the sandbox)

Cosmetic changes

60456c2

protoc still not working

Get common proto files in a box

ff2b117

Process top-level .include files and directories

508e6c2

Allow include files anywhere in acceptance/ hierarchy into sandbox

85188ef

Better trace the assets read in

1389599

Check the generation step succeded via zero exit code

2acf5c1

In the future, we'll be able to configure expected failures via a config file, and check for those as well. This also includes miscellaneous documentation improvements.

Make the generation step run with samplegen turned on

238659c

In the process, constructed a sample config for the echo service and made the hard-coded generator default be protoc-gen-python_gapic via protoc.

Move qualify to be a gapic-showcase subcommand

27f00d9

This creates a package `qualifier`

Remove debugging comments

b773222

Allow passing flags and args to the qualify command

8fbe238

Run showcase servers in process when qualifying

242179f

This involved refactoring the "run" command so that we can start a server and not block on it ending.

Check for python plugin in prepare-to-qualify

d34b2aa

Make Packr part of the release process.

44447c3

We now officially bundle the acceptance files within the Go binary.

vchudnov-g added 6 commits October 14, 2019 16:24

Remove startShowcase() and endProcess()

34cb76b

Add sanity check in case the assets are empty

9265b41

Otherwise we'll trivially succeed on zero checks.

Remove getAllFiles

36a69f8

Document code and add licenses

25eb747

Update go.mod and go.sum after rebasing to latest main/HEAD

c80a225

Rename the demo test suite so it's called "basic-check"

a7c1bf7

vchudnov-g added the bug label Oct 14, 2019

vchudnov-g requested a review from noahdietz October 14, 2019 23:54

vchudnov-g self-assigned this Oct 14, 2019

vchudnov-g added enhancement and removed bug labels Oct 14, 2019

Minor cosmetic tweaks

a41706a

vchudnov-g commented Oct 15, 2019

View reviewed changes

vchudnov-g commented Oct 16, 2019

View reviewed changes

noahdietz reviewed Oct 16, 2019

View reviewed changes

vchudnov-g added 2 commits October 21, 2019 10:52

Code review changes: code

46f760f

Give the example includes more descriptive names

fe47602

And include a README about them

vchudnov-g commented Oct 21, 2019

View reviewed changes

google-cloud-label-sync bot removed the enhancement label Aug 28, 2020

parthea marked this pull request as draft April 17, 2024 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of GAPIC generator acceptance harness #222

Initial implementation of GAPIC generator acceptance harness #222

vchudnov-g commented Oct 14, 2019 •

edited

codecov bot commented Oct 14, 2019 •

edited

vchudnov-g Oct 15, 2019

noahdietz Oct 16, 2019

vchudnov-g Oct 19, 2019

vchudnov-g Oct 19, 2019

noahdietz Oct 21, 2019

noahdietz Oct 21, 2019 •

edited

vchudnov-g Oct 16, 2019

noahdietz Oct 16, 2019

vchudnov-g Oct 21, 2019

noahdietz Oct 21, 2019

vchudnov-g Nov 6, 2019

noahdietz Nov 6, 2019

noahdietz left a comment

noahdietz Oct 16, 2019

noahdietz Oct 16, 2019

noahdietz Oct 16, 2019

vchudnov-g Oct 21, 2019

noahdietz Oct 16, 2019

vchudnov-g Oct 19, 2019

vchudnov-g left a comment

vchudnov-g Oct 19, 2019

vchudnov-g Oct 19, 2019

vchudnov-g Oct 21, 2019

vchudnov-g Oct 21, 2019

noahdietz commented Oct 22, 2019

vchudnov-g commented Oct 22, 2019

noahdietz commented Oct 22, 2019

Initial implementation of GAPIC generator acceptance harness #222

Are you sure you want to change the base?

Initial implementation of GAPIC generator acceptance harness #222

Conversation

vchudnov-g commented Oct 14, 2019 • edited

codecov bot commented Oct 14, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahdietz Oct 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahdietz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahdietz commented Oct 22, 2019

vchudnov-g commented Oct 22, 2019

noahdietz commented Oct 22, 2019

vchudnov-g commented Oct 14, 2019 •

edited

codecov bot commented Oct 14, 2019 •

edited

noahdietz Oct 21, 2019 •

edited