tests: flag grpc sandbox for unit tests

[callmehiphop]

Closes #1140

We've been seeing some test failures since moving PubSub to gRPC. This PR will stop any gRPC calls from actually being made in our unit tests.

[callmehiphop]

@stephenplusplus I'm not really sure if this is the way to go, so your guidance is very welcome here :)

[stephenplusplus]

Has this made a difference locally?

[callmehiphop]

Yep, locally I'm all green with this.

[stephenplusplus]

What was the error that was happening without this change?

[callmehiphop]

We're seeing consistent failures with v0.12.x in regards to currentRetry. I noticed this when reviewing both #1146 and #1147.

https://travis-ci.org/GoogleCloudPlatform/gcloud-node/jobs/113138184

[callmehiphop]

Related: #1140

[jgeewax]

Is this an issue with the PubSub gRPC service? or something about our testing environment?

Ideally.. our tests run both gRPC and the non-gRPC paths so we know if there are problems.... right ?

[stephenplusplus]

Yes, I sadly know about that currentRetry thing-- but that's from the Mocha library, and I really don't know what's causing it. gRPC already shouldn't be running without the change from this PR.

[callmehiphop]

Is this an issue with the PubSub gRPC service? or something about our testing environment?

Looks to be our testing environment.

gRPC already shouldn't be running without the change from this PR.

Locally I was also receiving this error

1) PubSub subscribe should create a Topic object from a string:
   Uncaught Error: .google.pubsub.v1.Subscription#a is not a field: undefined
      at Error (native)
      at MessagePrototype.set (node_modules/grpc/node_modules/protobufjs/dist/ProtoBuf.js:2440:58)
      at MessagePrototype.set (node_modules/grpc/node_modules/protobufjs/dist/ProtoBuf.js:2434:38)
      at Message (node_modules/grpc/node_modules/protobufjs/dist/ProtoBuf.js:2363:34)
      at serialize (node_modules/grpc/src/node/src/common.js:76:23)
      at Client.makeUnaryRequest [as createSubscription] (node_modules/grpc/src/node/src/client.js:303:19)
      at PubSub.GrpcService.request (lib/common/grpc-service.js:214:28)
      at lib/common/grpc-service.js:187:12
      at lib/common/grpc-service.js:285:5
      at addScope (node_modules/google-auto-auth/index.js:74:5)
      at callback (node_modules/google-auto-auth/node_modules/google-auth-library/lib/auth/googleauth.js:41:5)
      at node_modules/google-auto-auth/node_modules/google-auth-library/lib/auth/googleauth.js:124:9

Also after flagging GCLOUD_SANDBOX_ENV, Travis is green again - https://travis-ci.org/GoogleCloudPlatform/gcloud-node/builds/113163438

[stephenplusplus]

Locally I was receiving this error

Okay, lets just patch that hole up. We shouldn't be letting requests slip through to GrpcService.

[jgeewax]

Sorry, just to make sure I understand -- these are "should be mocked" methods, right? For system tests, do we send live requests both via HTTP/1.1 and gRPC?

[stephenplusplus]

For system tests, do we send live requests both via HTTP/1.1 and gRPC?

In our system tests, we send real requests through the gRPC library, which does... whatever it does. In unit tests, we mock requests. But @callmehiphop caught a place where we were letting a unit test slip through to the real API.

[jgeewax]

SGTM. Ignore the noise. :)

[stephenplusplus]

1. PubSub subscribe should create a Topic object from a string:
   Uncaught Error: .google.pubsub.v1.Subscription#a is not a field: undefined

Since I haven't been able to reproduce this error locally, I don't really know how to fix it. Maybe if we didn't actually inherit from the class with our FakeGrpcService, it wouldn't have a chance to call through to the real one: https://github.com/GoogleCloudPlatform/gcloud-node/blob/b55b049b6bc98d807da2228c3cb60a95efda89f4/test/pubsub/index.js#L39-L44

[callmehiphop]

Maybe if we didn't actually inherit from the class with our FakeGrpcService, it wouldn't have a chance to call through to the real one

Isn't the reason GCLOUD_SANDBOX_ENV exists to prevent gRPC calls from going through in the doc unit tests? I'm not sure I understand why we need another mechanism to accomplish the same thing.

[stephenplusplus]

Just unit testing principles. We shouldn't have leaks in our unit tests.
The docs situation is kind of magical how we pull that off, so we have to
create a sandbox environment for that code to run in. We aren't able to
mock in that situation, which is why that global has to sadly exist.

On Thu, Mar 3, 2016, 1:12 PM Dave Gramlich notifications@github.com wrote:

Maybe if we didn't actually inherit from the class with our
FakeGrpcService, it wouldn't have a chance to call through to the real one

Isn't the reason GCLOUD_SANDBOX_ENV exists to prevent gRPC calls from
going through in the doc unit tests? I'm not sure I understand why we need
another mechanism to accomplish the same thing.

—
Reply to this email directly or view it on GitHub
#1149 (comment)
.

[callmehiphop]

Fair enough, I'll close this PR then!

[stephenplusplus]

Okay, are you going to try to fix the leak you found? I'm kind of leaning on you for that, since I can't reproduce.

[callmehiphop]

I hadn't planned on it, I believe that error is directly related to #1140 which still needs fixing. Have you tried testing with v0.12.7?