Refactor rate converter separating scheduler from converter logic to improve testability #1394

bsardo · 2020-07-13T15:52:34Z

No description provided.

currencies/ticker_task_test.go

SyntaxNode · 2020-07-14T17:54:56Z

clock/clock.go

+	"time"
+)
+
+type Clock interface {


I have a few recommendations:

The code in this PR is only using the Now method. I recommend removing the Since and Until methods from the interface to keep things simple.

Since we're used to seeing time.Now(), consider a name closer to that.. such as the Time interface.

Consider moving this code to the util package. The packages with 'top billing' help describe the main functions of the app. I don't think this warrants a visible top level billing.

... after those changes, I don't think we need to credit the glock repo.

SyntaxNode · 2020-07-14T17:57:19Z

clock/mock_clock.go

@@ -0,0 +1,69 @@
+// parts copied from: https://github.com/efritz/glock
+
+package clock


I think we can remove this mock entirely. It's a simple enough interface to create a simple 'static clock' or some such where it's needed. The use of a mutex and the mock tests is overkill for our use case, IMHO.

SyntaxNode · 2020-07-14T17:58:11Z

currencies/rate_converter.go

 	updateNotifier      chan<- int
 	fetchingInterval    time.Duration
 	staleRatesThreshold time.Duration
 	syncSourceURL       string
 	rates               atomic.Value // Should only hold Rates struct
 	lastUpdated         atomic.Value // Should only hold time.Time
 	constantRates       Conversions
+	pbsClock            clock.Clock


There's no need to prefix with pbs. A simple name like clock would be preferred.

SyntaxNode · 2020-07-14T17:58:44Z

currencies/rate_converter.go

 ) *RateConverter {
 	return NewRateConverterWithNotifier(
 		httpClient,
 		syncSourceURL,
 		fetchingInterval,
 		staleRatesThreshold,
+		pbsClock,


I think you can hardcode a RealClock here. We only really need to inject a fake clock for the unit tests.

SyntaxNode · 2020-07-14T17:59:19Z

currencies/rate_converter.go

 		updateNotifier:      updateNotifier,
 		fetchingInterval:    fetchingInterval,
 		staleRatesThreshold: staleRatesThreshold,
 		syncSourceURL:       syncSourceURL,
 		rates:               atomic.Value{},
 		lastUpdated:         atomic.Value{},
 		constantRates:       NewConstantRates(),
+		pbsClock:            pbsClock,


There's no need to prefix with pbs. A simple name like clock would be preferred.

SyntaxNode · 2020-07-14T19:52:04Z

currencies/ticker_task.go

+		case <-tt.done:
+			if ticker != nil {
+				ticker.Stop()
+				ticker = nil


There's no need to set the ticker to nil. Once the loop ends, the ticker is out of scope.

SyntaxNode · 2020-07-14T19:56:22Z

currencies/ticker_task_test.go

+func TestStop(t *testing.T) {
+	// Setup:
+	calledURLs := []string{}
+	mockedHttpServer := httptest.NewServer(http.HandlerFunc(


Now that this is only testing TickerTask, I recommend removing everything related to the currency converter to keep the setup small.

SyntaxNode · 2020-07-14T20:00:24Z

currencies/ticker_task_test.go

+	// Execute:
+	expectedTicks := 2
+	ticks := make(chan int)
+	fetchingInterval := time.Duration(100) * time.Millisecond


Yeah, I agree there's no way to remove the dependence on the real system clock without also injecting a mock timer. If we have to keep with this approach, please use a much shorter interval. 100ms is way to long for a unit test IMHO, where 1ms should be fine. Similarly, the 1 second sleep is extremely too long for a unit test.

Stopping after the second tick seems fine. Then you sleep for another 2ms perhaps to ensure the timer has stopped and assert the count is still 2.

SyntaxNode · 2020-07-14T20:01:49Z

exchange/bidder_test.go

 		)
+		time.Sleep(time.Duration(500) * time.Millisecond)
+		currencyConverterTickerTask := currencies.NewTickerTask(time.Duration(10)*time.Second, currencyConverter)


What is this test testing? I don't see why it needs a timer running.

This test is verifying the requestBid function when provided with valid rate conversions. We don't need to run the timer here. We can set up our conversions by creating a new rate converter, call the converter method Run() once so that the mocked rates are stored in the converter state and then calling Rates() on the converter to retrieve the conversions to be passed into requestBid.

exchange/bidder_test.go

stale · 2020-07-29T05:40:14Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

SyntaxNode · 2020-07-30T19:44:19Z

currencies/rate_converter.go

 	fetchingInterval    time.Duration
 	staleRatesThreshold time.Duration
 	syncSourceURL       string
 	rates               atomic.Value // Should only hold Rates struct
 	lastUpdated         atomic.Value // Should only hold time.Time
 	constantRates       Conversions
+	clock               timeutil.Time


Consider renaming clock to time to match.

SyntaxNode · 2020-07-30T19:44:49Z

currencies/rate_converter.go

@@ -55,30 +59,20 @@ func NewRateConverterWithNotifier(
 	syncSourceURL string,
 	fetchingInterval time.Duration,
 	staleRatesThreshold time.Duration,
+	clock timeutil.Time,


Consider renaming clock to time to match.

SyntaxNode · 2020-07-30T19:46:58Z

currencies/rate_converter.go

@@ -93,6 +87,10 @@ func (rc *RateConverter) fetch() (*Rates, error) {
 		return nil, err
 	}

+	if response.StatusCode >= 400 {
+		return nil, &errortypes.BadServerResponse{Message: "error"}


Let's get a bit more descriptive than "error". Possibly include the status code.

SyntaxNode · 2020-07-30T19:48:30Z

currencies/ticker_task.go

@@ -0,0 +1,61 @@
+package currencies


Let's move this to the util\task package or something like that.

SyntaxNode · 2020-07-30T19:51:19Z

currencies/ticker_task.go

+
+type Runner interface {
+	Run() error
+	Notify()


Let's delete the notifier. Looks like it's just used for tests.

SyntaxNode · 2020-07-30T19:59:13Z

util/timeutil/real_clock.go

+	return time.Now()
+}
+
+func (c *RealClock) Sleep(duration time.Duration) {


No longer used. Please remove.

SyntaxNode · 2020-07-30T20:00:36Z

currencies/rate_converter_test.go

-	)
-
-	defer mockedHttpServer.Close()
+type MockClock struct {


Consider naming this FakeClock since it doesn't have any mocking behavior and change fakeTime to just time.

SyntaxNode · 2020-07-30T20:13:23Z

currencies/ticker_task_test.go

+}
+
+func (mcc *MockRunner) Run() error { return nil }
+func (mcc *MockRunner) Notify()    { mcc.runCount++; mcc.updateNotifier <- mcc.runCount }


Let's remove Notify and have the Run method increase the count for the test to inspect.

SyntaxNode · 2020-07-30T20:14:03Z

currencies/ticker_task_test.go

+	// Setup:
+	ticks := make(chan int)
+	mockRunner := &MockRunner{updateNotifier: ticks}
+	interval := time.Duration(1) * time.Millisecond


Can this be simplified as 1 * time.Millisecond.

SyntaxNode · 2020-07-30T20:16:04Z

currencies/ticker_task_test.go

+
+	// Execute:
+	interval := time.Duration(0)
+	currencyConverter := currencies.NewRateConverter(


No need to use the NewRateConverter in this test.

SyntaxNode · 2020-08-03T20:29:18Z

currencies/rate_converter.go

-func NewRateConverterWithNotifier(
+// NewConfiguredRateConverter returns a new RateConverter
+// Do not call this method directly; it is to improve testability only
+func NewConfiguredRateConverter(


Do we need the separate constructor? The purpose for the time dependency is to enable unit tests. The unit tests can construct a rate converter normally and then change the time to a mocked version since the tests are part of the same package.. or just omit the constructor entirely and directly set the struct for testing.

That's a good point. I was trying to keep time private but you're right, we only added it to enable UTs so I should just make it public and then we can overwrite it in tests.

You don't need to make it public for the unit tests. Access modifiers in Go are only at the package level and unit tests run within the same package.

Yeah I could make it private but I wanted to continue following the practice of testing the module from a _test package to ensure we are only touching the API and publicly accessible fields.

SyntaxNode · 2020-08-03T20:31:49Z

util/task/ticker_task.go

+
+// run creates a ticker that ticks at the specified interval. On each tick,
+// the task is executed
+func (t *TickerTask) run() {


Consider renaming to runRecurring to better differentiate from the t.runner.Run() call.

SyntaxNode · 2020-08-03T20:38:03Z

currencies/rate_converter_test.go

+
+	// Verify rates are valid and last update ts is correct
+	assert.Equal(t, expectedRates, currencyConverter.Rates(), "Conversions.Rates weren't the expected ones")
+	assert.Equal(t, initialFakeTime, currencyConverter.LastUpdated(), "LastUpdated should be set")
 }

 func TestRace(t *testing.T) {


I guess we can leave the logic alone. I don't follow it's design. 50 clients seems like a lot and the random intervals only decrease the change of a race condition and lower the effectiveness of the test. Since order of operations will be impossible to predict here, this looks like a stub for Go's memory race detector to report on unprotected access.

Yeah I don't understand it either. I took a look at the issue that resulted in this test but I still hesitated to edit this because I was afraid I would be removing coverage of some edge case I'm not seeing. I'll take a stab at writing a simpler race test given it seems overcomplicated to both of us.

guscarreon · 2020-08-06T19:18:49Z

util/task/ticker_task_test.go

+	// Setup:
+	runner := &MockRunner{RunCount: 0}
+	interval := 0 * time.Millisecond
+	ticker := task.NewTickerTask(interval, runner)


TestStop does make a ticker.Stop() call. Are we missing that call here?

We shouldn't need it here because when Start is called, the interval is zero so the go routine that instantiates the ticker is never started. Since it never starts, there is no need to send a signal on the done channel because we don't need to stop the ticker.
In TestStop, we are testing the Stop method which is why it is called. Also, in that test the go routine starts and the ticker is instantiated.

This is perhaps a bit of overkill for the tests, but if we think it would prevent issues then you have my support.

That's a very good point though. We we properly handling graceful shutdowns? When PBS receives a quit signal from the OS, are we stopping the fetchers?

I can open a story to ensure we are gracefully shutting down.

I have seen the shutdown pattern in fetchers and I was thinking that since those goroutines maintain no external state to gracefully sync, are there any drawbacks to just letting them be terminated ? Their connections are going to be closed when the process ends anyway, and that seems to be the only time this happens. We don't reload configs, where this could be useful.

Perhaps it might not be needed. We created an internal story to check if there's a need for a graceful shutdown.

guscarreon · 2020-08-06T19:19:05Z

util/task/ticker_task_test.go

+
+	// Execute:
+	ticker.Start()
+	time.Sleep(25 * time.Millisecond)


TestStop does make a ticker.Stop() call. Are we missing that call here?

Adding Stop() here makes sense as it would lead to more consistent test results. However, I think we can just combine this test with TestStop since testing the stop method is dependent on a task running periodically.

I vote to leave the tests separate. TestStartWithPeriodicRun is validating an expected run count and adding Stop() is just for cleanup. The TestStop test has additional wait time to ensure the stop call actually worked. Leaving them separate should point us more clearly to a potential issue.

👍 I'll leave the two tests separate and add Stop().

guscarreon · 2020-08-06T19:20:18Z

main.go

+	currencyConverter := currencies.NewRateConverter(&http.Client{}, cfg.CurrencyConverter.FetchURL, fetchingInterval, staleRatesThreshold)
+
+	currencyConverterTickerTask := task.NewTickerTask(fetchingInterval, currencyConverter)
+	currencyConverterTickerTask.Start()


I'm not sure if its necessary but, should we also add defer currencyConverterTickerTask.Stop()?

I don't think it's necessary; Stop() is just closing a channel which halts the timer. I guess I don't see any risk in an extra tick or two occurring while in the process of a shutdown, especially since the ticker is just being used for rate conversions at this time.

From the golang docs: "Channels aren't like files; you don't usually need to close them. Closing is only necessary when the receiver must be told there are no more values coming, such as to terminate a range loop."

Also deferred calls don't run when the main go routine exits. I think we would need to use sync waitgroups for this sort of thing.

guscarreon · 2020-08-06T20:50:09Z

currencies/rate_converter.go

+// By default there will be no currencies conversions done.
+// `currencies.ConstantRate` will be used.
+func NewRateConverterDefault() *RateConverter {
+	return NewRateConverter(&http.Client{}, "", time.Duration(0), time.Duration(0))
 }


This function seems to not be used anywhere else but in _test.go after your refactor. Do you think we can safely get rid of it?

I don't think this function was ever used in production code. I deleted it and replaced the calls with calls to NewRateConverter.

…age or needed for testing

Simplify clock interface and implementation Simplify variable names Further isolate ticker task TestStop test by using mock runner instead of rate converter instance

…ogic is task specific

SyntaxNode self-requested a review July 14, 2020 03:35

SyntaxNode self-assigned this Jul 14, 2020

bsardo commented Jul 14, 2020

View reviewed changes

currencies/ticker_task_test.go Outdated Show resolved Hide resolved

SyntaxNode requested a review from guscarreon July 14, 2020 14:11

SyntaxNode assigned guscarreon Jul 14, 2020

SyntaxNode reviewed Jul 15, 2020

View reviewed changes

stale bot added the stale label Jul 29, 2020

SyntaxNode reviewed Jul 30, 2020

View reviewed changes

stale bot removed the stale label Jul 30, 2020

bsardo requested a review from SyntaxNode August 3, 2020 12:32

SyntaxNode reviewed Aug 3, 2020

View reviewed changes

bsardo requested a review from SyntaxNode August 4, 2020 14:34

SyntaxNode previously approved these changes Aug 4, 2020

View reviewed changes

bsardo mentioned this pull request Aug 6, 2020

Add support for Account configuration (PBID-727, #1395) #1426

Merged

guscarreon reviewed Aug 6, 2020

View reviewed changes

bsardo dismissed SyntaxNode’s stale review via 061aad1 August 7, 2020 15:07

bsardo requested review from guscarreon and SyntaxNode August 7, 2020 15:09

bsardo added 9 commits August 7, 2020 12:04

Separate rate converter logic from periodic task scheduling.

9bbed41

Refactor rate converter splitting the ticker from the converter logic

cc006d0

Move clock interfaces and real clock to their own files

32b95b8

Remove unused clock methods

ef89867

Only export rate converter methods needed outside the currencies pack…

7ad09b0

…age or needed for testing

Add mock clock tests

b8f6d36

Move clock files into clock package

2b638ad

Move clock package to timeutil package

abad978

Simplify clock interface and implementation Simplify variable names Further isolate ticker task TestStop test by using mock runner instead of rate converter instance

Move ticker task channel notifier onto the task Runner interface so l…

6afc23f

…ogic is task specific

		@@ -0,0 +1,69 @@
		// parts copied from: https://github.com/efritz/glock

		package clock

Refactor rate converter separating scheduler from converter logic to improve testability #1394

Refactor rate converter separating scheduler from converter logic to improve testability #1394

Conversation

bsardo commented Jul 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

SyntaxNode Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxNode Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo Aug 6, 2020 •

edited

Loading

SyntaxNode Aug 6, 2020 •

edited

Loading

SyntaxNode Aug 12, 2020 •

edited

Loading