expose Config.Test with Time setting

[bgentry]

The new TestConfig type will contain test-specific client configs. For now, its only setting is a Time field which contains a TimeGenerator type that can be used to set a synthetic clock for testing.

As part of this, the TimeGenerator interface was moved from the semi-internal rivershared package to rivertype for the sake of API guarantees. This will require updates in dependent packages (riverpro).

Fixes #751.

[brandur]

The only other thing that comes to mind is that it feels a little weird to make people write their own TimeGenerator implementations every time they used the project. Should we move TimeStub into rivertest do you think?

[brandur]

It feels a little off to require this as part of the interface — it's basically there so we can do shortcuts in tests like this:

		archetype := riversharedtest.BaseServiceArchetype(t)
		archetype.Time.StubNowUTC(time.Now().UTC())

But unlike our test code, users won't be able to do that anyway because they won't have access to the TimeGenerator on a River client after they've set it (archetype is not exported).

We could potentially fix this by breaking TimeGenerator into two related interfaces along with a wrapper like:

diff --git a/client.go b/client.go
index 74643ad..06c9533 100644
--- a/client.go
+++ b/client.go
@@ -510,7 +510,11 @@ func NewClient[TTx any](driver riverdriver.Driver[TTx], config *Config) (*Client
 
 	archetype := baseservice.NewArchetype(config.Logger)
 	if config.time != nil {
-		archetype.Time = config.time
+		if withStub, ok := config.time.(baseservice.TimeGeneratorWithStub); ok {
+			archetype.Time = withStub
+		} else {
+			archetype.Time = &baseservice.TimeGeneratorWithStubWrapper{TimeGenerator: config.time}
+		}
 	}
 
 	client := &Client[TTx]{
diff --git a/rivershared/baseservice/base_service.go b/rivershared/baseservice/base_service.go
index 764c0f3..d5b1fe2 100644
--- a/rivershared/baseservice/base_service.go
+++ b/rivershared/baseservice/base_service.go
@@ -25,7 +25,7 @@ type Archetype struct {
 	// `time.Now().UTC()`, but is riverinternaltest.timeStub in tests to allow
 	// the current time to be stubbed.  Services should try to use this function
 	// instead of the vanilla ones from the `time` package for testing purposes.
-	Time TimeGenerator
+	Time TimeGeneratorWithStub
 }
 
 // NewArchetype returns a new archetype. This function is most suitable for
@@ -90,12 +90,28 @@ type TimeGenerator interface {
 	// where a component may want to use a stubbed time if the time is stubbed,
 	// but to fall back to a database time default otherwise.
 	NowUTCOrNil() *time.Time
+}
+
+type TimeGeneratorWithStub interface {
+	TimeGenerator
 
 	// StubNowUTC stubs the current time. It will panic if invoked outside of
 	// tests. Returns the same time passed as parameter for convenience.
 	StubNowUTC(nowUTC time.Time) time.Time
 }
 
+// TimeGeneratorWithStubWrapper provides a wrapper around TimeGenerator that
+// implements missing TimeGeneratorWithStub functions. This is used so that we
+// only need to expose the minimal TimeGenerator interface publicly, but can
+// keep a stubbable version of widely available for internal use.
+type TimeGeneratorWithStubWrapper struct {
+	TimeGenerator
+}
+
+func (g *TimeGeneratorWithStubWrapper) StubNowUTC(nowUTC time.Time) time.Time {
+	panic("time not stubbable outside tests")
+}
+
 // UnStubbableTimeGenerator is a TimeGenerator implementation that can't be
 // stubbed. It's always the generator used outside of tests.
 type UnStubbableTimeGenerator struct{}

Thoughts?

[bgentry]

All great ideas, thank you! Applied them all in the latest revision.

[brandur]

lgtm.

added a rivertest.TimeStub implementation for public consumption, couldn't remove the old one though due to import cycles and rivertest referencing all other internal packages in one way or another.

[bgentry]

We could just call this Stub? It doesn't even need to match the internal baseservice.TImeGeneratorWithStub because there's a circular dependency and that one has to exist separately. Your call!

[brandur]

Hm, unless we're going to rename the other two functions, I'd probably leave it. Just seems more consistent.

[brandur]

Hm, unless we're going to rename the other two functions, I'd probably leave it. Just seems more consistent.