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

test: end-to-end system test #471

Merged
merged 7 commits into from Apr 26, 2019
Merged

test: end-to-end system test #471

merged 7 commits into from Apr 26, 2019

Conversation

alexander-fenster
Copy link
Member

This PR adds a Real Life gRPC test for gax.

I take gapic-showcase, which shows all possible GAPIC features, start a local gRPC server and run all methods of the Echo service from Showcase against this gRPC server (using GAPIC-generated EchoClient).

The idea of this system test is to check that regular GAPIC client (the same as all our client libraries) tests all GAX special features (streaming, paging, long running operations).

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 25, 2019
@alexander-fenster
Copy link
Member Author

Note: I'm currently using my temporary gRPC server implementation in TypeScript. It will eventually be replaced with the 'official' Go implementation of Showcase gRPC server (there is nothing specifically technical about that, just some work with CI scripts to properly download and run the server for the right platform is required). Until this is done, let's use my TypeScript server placeholder.

"license": "Apache-2.0",
"keywords": [],
"scripts": {
"lint": "eslint src/ samples/ system-test/ test/",
Copy link
Contributor

Choose a reason for hiding this comment

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

For both of these, I've had a lot better luck just doing eslint '**/*.js'.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

"gts": "^1.0.0-0",
"mocha": "^6.1.4",
"prettier": "^1.17.0",
"through2": "^3.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Really question why you need through2, it is mostly the same thing as new PassThrough but with a lot less dependencies

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sure I don't need it and it will be taken care of in the micro-generator but I just decided not to change anything here in the test app.

"eslint-config-prettier": "^4.1.0",
"eslint-plugin-node": "^8.0.1",
"eslint-plugin-prettier": "^3.0.1",
"gts": "^1.0.0-0",
Copy link
Contributor

Choose a reason for hiding this comment

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

just 1.0.0 now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not TypeScript (yet) :( It's a leftover. No need to have any kind of gts here.

"@grpc/grpc-js": "^0.3.6",
"google-gax": "file:./google-gax.tgz",
"grpc": "^1.20.0",
"lodash.merge": "^4.6.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we carefully looked at why we need lodash.merge here? Are we doing something Object.assign can't?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need lodash.merge, micro-generators will emit Object.assign. https://github.com/googleapis/gapic-generator-typescript/blob/0080e041a9488a8976c94464e2cd75c25e9e1107/src/v1alpha3/echo_client.ts#L96 Again, this is just a generated code, which I will eventually replace with TypeScript generated code (when it's ready).

"mocha": "^6.1.4",
"prettier": "^1.17.0",
"through2": "^3.0.1",
"typescript": "~3.3.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Up to 3.4.0 now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

... but not needed here (yet) at all, so removed.

@@ -0,0 +1,8 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

why are the two prettier configs in this PR different?

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad! Fixed.


// Helper functions to be used from the system tests

import * as cp from 'child_process';
Copy link
Contributor

Choose a reason for hiding this comment

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

feels like we could save a lot of boilerplate here by just using execa

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmmm.

@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #471   +/-   ##
=======================================
  Coverage   88.88%   88.88%           
=======================================
  Files           1        1           
  Lines         333      333           
=======================================
  Hits          296      296           
  Misses         37       37

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 b7ea19a...ef7dc9a. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #471   +/-   ##
=======================================
  Coverage   88.88%   88.88%           
=======================================
  Files           1        1           
  Lines         333      333           
=======================================
  Hits          296      296           
  Misses         37       37

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 b7ea19a...ef7dc9a. Read the comment docs.

@alexander-fenster alexander-fenster merged commit 55b4d3b into master Apr 26, 2019
@alexander-fenster alexander-fenster deleted the real-grpc-test branch April 26, 2019 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants