feat(pubsub): Add pubsub.AddCallbackForTesting()

[Jille]

Add pubsub.AddCallbackForTesting()

Otherwise crafting a *pubsub.Message panics when Ack() is called.

[hongalex]

Could you clarify what you mean by "crafting a *pubsub.Message panics when Ack() is called"?
Are you using this in conjunction with the pstest fake?

[Jille]

No, I didn't know of the pstest package. My test had:

m := pubsub.Message{Data: d}
handleMyPubsubMessage(ctx, m)

and my code-under-test:

func handleMyPubsubMessage(ctx context.Context, m *pubsub.Message) {
  // ...
  m.Ack()
}

[hongalex]

This proposed change replaces the client populated doneFunc (from iterator.go), which is concerning to me. In general, we want to keep the API surface small and simple so as to avoid user confusion. The doneFunc is private for the reason that we don't want users manipulating acknowledge.

I believe a simpler solution is to guard against calling the doneFunc (with if m.doneFunc != nil, so that users can still test with exposed Message object.

[Jille]

With if m.doneFunc != nil, how would you propose people test whether their code calls Ack (at the right moment)?

[tbpg]

Hi. Unfortunately, I'm a -1 on this. There are other ways to achieve this behavior without adding to the API surface. Please check out our new testing guide here: https://github.com/googleapis/google-cloud-go/blob/master/testing.md.

[hongalex]

how would you propose people test whether their code calls Ack (at the right moment)?

The interesting thing is that Ack always succeeds. This is true for both the message.Ack function as well as the Acknowledge RPC. More specifically, all message.Ack does is adding the ackID to a queue that eventually calls Acknowledge on a timer. If you really want to test that an Ack is received by the server, you would need to instantiate a fake server as tbpg@ mentioned. This is more of an end-to-end test since you'll have to actually receive a message from the fake server as well.

[Jille]

@tbpg, that seems suitable for end-to-end tests, not for unit tests.

@hongalex, I don't want to test whether the Ack is received. I just want to test whether it's called. That seems like the right scope for a unit test, no?

I sent #3139 to unblock myself and just live crappier testing :(

[hongalex]

We're actually looking to expose user defined ack/nack handlers for a message. This was requested recently as part of the design of the pubsub lite library which will share the same pubsub.Message struct. I'm not certain on the timing of this, but I'll approve and merge you current PR in the meantime and update you once the custom ack handler is added in.

Thanks for your patience!

[tbpg]

One idea is to define an interface with the functions you need, one of which is Ack. Then, while testing, you can use a test implementation of that interface where you can confirm Ack is called. Unfortunately, I still don't think we should extend the exported API for this.

Another idea is to create your own client type that wraps the cloud.google.com/go/pubsub.Client type. Then you can add whatever hooks you need in various functions and you don't need test/prod implementations.

[hongalex]

Closing in favor of #3200