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

Please make a LocalPubSubHelper like LocalDatastoreHelper #2084

Closed
rcleveng opened this issue May 23, 2017 · 9 comments
Closed

Please make a LocalPubSubHelper like LocalDatastoreHelper #2084

rcleveng opened this issue May 23, 2017 · 9 comments
Assignees
Labels
api: pubsub Issues related to the Pub/Sub API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@rcleveng
Copy link

rcleveng commented May 23, 2017

(This is an enhancement request)

I think the emulator would need to handle a /shutdown request and then you could do something very similar to what the datastore does.

Then tests can start/stop it without needing to run the emulator 1st.

See https://github.com/GoogleCloudPlatform/google-cloud-java/blob/1c456ead3455680ef198900712ff481d634ec530/google-cloud-datastore/src/main/java/com/google/cloud/datastore/testing/LocalDatastoreHelper.java

@vam-google vam-google added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. priority: p2 Moderately-important priority. Fix may not be included in next release. labels May 23, 2017
@vam-google
Copy link
Contributor

@pongad please take a look. For PubSub tests we currently don't have a Local*Helper class, like we do for several others (storage, datastore). The tests currently create fake services instead on @Before call for similar purpose (like in SubscriberTest.setUp() method).

@rcleveng
Copy link
Author

Looking at this, it would seem we should use ServiceOptions and the rest of the common classes for creating the publisher,subscriber too.

@vam-google vam-google added the api: pubsub Issues related to the Pub/Sub API. label May 24, 2017
@pongad
Copy link
Contributor

pongad commented May 25, 2017

@garrettjonesgoogle is probably more opinionated about ServiceOptions than I am. The ServiceOptions were written for handwritten clients. A part of pubsub surface is machine generated that does not use these options. IIRC, when we created Publisher and Subscriber, we feared that the difference is too stark.

As for the local helpers, it was removed for a few reasons. I think the two main reasons are the following:
In the original form, we feel it reached too far behind the user. A test, I think, shouldn't be able to pull the emulator off the internet and run it.

Another is that process control of the emulator was extremely error prone. It was unclear how to properly expose emulator logs to the user, and starting and stopping the emulator was problematic, resulting in many deadlocks. My own thinking is that something like this is much easier (not tested but shouldn't be far off):

gcloud beta emulators pubsub start &
sleep 3 # wait for emulator to start

# Sets the PUBSUB_EMULATOR_HOST variable
$(gcloud beta emulators pubsub env-init)

# run code

curl -XPOST $PUBSUB_EMULATOR_HOST/shutdown
wait # blocks until the emulator goes away

My fondness of shell is probably rather...outdated, but I think it solves many problems. With small modifications, you can process emulator logs, pipe to files, etc. Does this reasoning make any sense to you? Please let me know if this works at all for you.

@saturnism
Copy link

I feel the original tester overreached by downloading the emulator. But, launching an emulator on a random port, and providing that information back to the test client, was valuable.

I feel a good compromise is:

  1. A PubSubEmulator surface, that simply has start and stop methods. Its sole purpose is to issue gcloud emulators commands.
  2. Let the user control when to start/stop it

Or, another way is to containerize the emulator, and use testcontainers to setup the emulator.
https://www.testcontainers.org/usage/generic_containers.html#example

@garrettjonesgoogle
Copy link
Member

@saturnism , #1 still implies doing process control, and lifecycle issues around starting and stopping the emulator. @pongad 's recommended solution of starting and stopping the emulator in a shell script that also invokes the tests gives the users the control they need.

@mattnworb
Copy link

Sorry to revisit an old issue, but I'm in the midst of updating a codebase which uses google-cloud-pubsub:0.8.0 to more recent versions and which has a lot of tests that use LocalPubsubHelper.

It seems to me like the arguments here against having a helper that starts the emulator via gcloud (or downloads it) also apply for LocalDatastoreHelper, which still exists in this library.

FWIW we have never had any issues with the lifecycle control or the helper failing to kill an emulator, the two helpers have been a huge help for us and the removal of (the very useful) LocalPubsubHelper makes migrating to newer versions of the library difficult.

Have the thoughts around this changed at all in the 18 months or so since the issue was opened?

@yihanzhen
Copy link
Contributor

cc/ @chingor13

@elefeint
Copy link

elefeint commented Jan 4, 2019

As it happens, we've created an emulator test rule in spring-cloud-gcp.

It's easy to use, and takes care of its own set up and tear down.

@ClassRule
public static PubSubEmulator emulator = new PubSubEmulator();

We could move it to a less obscure location if it's useful.

@mattnworb
Copy link

@elefeint thanks, that looks useful!

I ended up using testcontainers to launch the emulator in a container in my series of tests, which I found easier than writing code to manage the gcloud process - and also has the nice benefit of only requiring that hosts running the tests have docker installed, rather than requiring gcloud and the emulator optional component to be installed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

8 participants