-
Notifications
You must be signed in to change notification settings - Fork 93
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
Conversation
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/", |
There was a problem hiding this comment.
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'
.
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 @@ | |||
--- |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad! Fixed.
system-test/util.ts
Outdated
|
||
// Helper functions to be used from the system tests | ||
|
||
import * as cp from 'child_process'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm.
Codecov Report
@@ 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.
|
1 similar comment
Codecov Report
@@ 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.
|
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).