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.

Sign up for GitHub

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

Remove time.Now() as the first parameter of NewRates() #1953

Merged
merged 2 commits into from
Aug 18, 2021
Merged

Remove time.Now() as the first parameter of NewRates() #1953

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffa3889
Remove time.Now() as the first parameter of NewRates()
Aug 10, 2021
60de4bb
Brian's review
Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions currency/aggregate_conversions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,21 @@ package currency
import (
"errors"
"testing"
"time"

"github.com/stretchr/testify/assert"
)

func TestGroupedGetRate(t *testing.T) {

// Setup:
customRates := NewRates(time.Now(), map[string]map[string]float64{
customRates := NewRates(map[string]map[string]float64{
"USD": {
"GBP": 3.00,
"EUR": 2.00,
},
})

pbsRates := NewRates(time.Now(), map[string]map[string]float64{
pbsRates := NewRates(map[string]map[string]float64{
"USD": {
"GBP": 4.00,
"MXN": 10.00,
Expand Down
3 changes: 0 additions & 3 deletions currency/rate_converter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ func TestReadWriteRates(t *testing.T) {
} else {
rates := currencyConverter.Rates().(*Rates)
assert.Equal(t, tt.wantConversions, (*rates).Conversions, tt.description)
assert.Equal(t, tt.wantDataAsOf, (*rates).DataAsOf, tt.description)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also be able to delete wantDataAsOf in the tests struct in this test TestReadWriteRates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're totally right. Removed

}

lastUpdated := currencyConverter.LastUpdated()
Expand All @@ -169,7 +168,6 @@ func TestRateStaleness(t *testing.T) {
defer mockedHttpServer.Close()

expectedRates := &Rates{
DataAsOf: time.Date(2018, time.September, 12, 0, 0, 0, 0, time.UTC),
VeronikaSolovei9 marked this conversation as resolved.
Show resolved Hide resolved
Conversions: map[string]map[string]float64{
"USD": {
"GBP": 0.77208,
Expand Down Expand Up @@ -257,7 +255,6 @@ func TestRatesAreNeverConsideredStale(t *testing.T) {
defer mockedHttpServer.Close()

expectedRates := &Rates{
DataAsOf: time.Date(2018, time.September, 12, 0, 0, 0, 0, time.UTC),
Conversions: map[string]map[string]float64{
"USD": {
"GBP": 0.77208,
Expand Down
26 changes: 1 addition & 25 deletions currency/rates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
package currency

import (
"encoding/json"
"errors"
"time"

"golang.org/x/text/currency"
)
Expand All @@ -12,38 +10,16 @@ import (
// note that `DataAsOfRaw` field is needed when parsing remote JSON as the date format if not standard and requires
// custom parsing to be properly set as Golang time.Time
type Rates struct {
DataAsOf time.Time `json:"dataAsOf"`
Conversions map[string]map[string]float64 `json:"conversions"`
}

// NewRates creates a new Rates object holding currencies rates
func NewRates(dataAsOf time.Time, conversions map[string]map[string]float64) *Rates {
func NewRates(conversions map[string]map[string]float64) *Rates {
return &Rates{
DataAsOf: dataAsOf,
Conversions: conversions,
}
}

// UnmarshalJSON unmarshal raw JSON bytes to Rates object
func (r *Rates) UnmarshalJSON(b []byte) error {
c := &struct {
DataAsOf string `json:"dataAsOf"`
Conversions map[string]map[string]float64 `json:"conversions"`
}{}
if err := json.Unmarshal(b, &c); err != nil {
return err
}

r.Conversions = c.Conversions

layout := "2006-01-02"
if date, err := time.Parse(layout, c.DataAsOf); err == nil {
r.DataAsOf = date
}

return nil
}

// GetRate returns the conversion rate between two currencies or:
// - An error if one of the currency strings is not well-formed
// - An error if any of the currency strings is not a recognized currency code.
Expand Down
87 changes: 25 additions & 62 deletions currency/rates_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,48 +2,31 @@ package currency

import (
"encoding/json"
"errors"
"testing"
"time"

"github.com/stretchr/testify/assert"
)

func TestUnMarshallRates(t *testing.T) {

// Setup:
testCases := []struct {
desc string
ratesJSON string
expectedRates Rates
expectsError bool
expectedError error
}{
{
ratesJSON: `{
"dataAsOf":"2018-09-12",
"conversions":{
"USD":{
"GBP":0.7662523901
},
"GBP":{
"USD":1.3050530256
}
}
}`,
expectedRates: Rates{
DataAsOf: time.Date(2018, time.September, 12, 0, 0, 0, 0, time.UTC),
Conversions: map[string]map[string]float64{
"USD": {
"GBP": 0.7662523901,
},
"GBP": {
"USD": 1.3050530256,
},
},
},
expectsError: false,
desc: "malformed JSON object, return error",
ratesJSON: `malformed`,
expectedRates: Rates{},
expectsError: true,
expectedError: errors.New("invalid character 'm' looking for beginning of value"),
},
{
desc: "Valid JSON field defining valid conversion object. Expect no error",
ratesJSON: `{
"dataAsOf":"",
"conversions":{
"USD":{
"GBP":0.7662523901
Expand All @@ -54,7 +37,6 @@ func TestUnMarshallRates(t *testing.T) {
}
}`,
expectedRates: Rates{
DataAsOf: time.Time{},
Conversions: map[string]map[string]float64{
"USD": {
"GBP": 0.7662523901,
Expand All @@ -64,47 +46,25 @@ func TestUnMarshallRates(t *testing.T) {
},
},
},
expectsError: false,
expectsError: false,
expectedError: nil,
},
{
desc: "Valid JSON field defines a conversions map with repeated entries, expect error",
ratesJSON: `{
"dataAsOf":"blabla",
"conversions":{
"USD":{
"GBP":0.7662523901
},
"GBP":{
"USD":1.3050530256
}
}
}`,
expectedRates: Rates{
DataAsOf: time.Time{},
Conversions: map[string]map[string]float64{
"USD": {
"GBP": 0.7662523901,
},
"GBP": {
"USD": 1.3050530256,
"GBP":0.7662523901,
"MXN":20.00
},
},
},
expectsError: false,
},
{
ratesJSON: `{
"dataAsOf":"blabla",
"conversions":{
"USD":{
"GBP":0.7662523901,
"GBP":0.7662523901
},
"GBP":{
"USD":1.3050530256,
}
}
}`,
expectedRates: Rates{},
expectsError: true,
expectedError: errors.New("invalid character '}' looking for beginning of object key string"),
},
}

Expand All @@ -114,15 +74,18 @@ func TestUnMarshallRates(t *testing.T) {
err := json.Unmarshal([]byte(tc.ratesJSON), &updatedRates)

// Verify:
assert.Equal(t, err != nil, tc.expectsError)
assert.Equal(t, tc.expectedRates, updatedRates, "Rates weren't the expected ones")
assert.Equal(t, err != nil, tc.expectsError, tc.desc)
if tc.expectsError {
assert.Equal(t, err.Error(), tc.expectedError.Error(), tc.desc)
}
assert.Equal(t, tc.expectedRates, updatedRates, tc.desc)
}
}

func TestGetRate(t *testing.T) {

// Setup:
rates := NewRates(time.Now(), map[string]map[string]float64{
rates := NewRates(map[string]map[string]float64{
"USD": {
"GBP": 0.77208,
},
Expand Down Expand Up @@ -165,7 +128,7 @@ func TestGetRate(t *testing.T) {
func TestGetRate_ReverseConversion(t *testing.T) {

// Setup:
rates := NewRates(time.Now(), map[string]map[string]float64{
rates := NewRates(map[string]map[string]float64{
"USD": {
"GBP": 0.77208,
},
Expand Down Expand Up @@ -219,7 +182,7 @@ func TestGetRate_ReverseConversion(t *testing.T) {
func TestGetRate_EmptyRates(t *testing.T) {

// Setup:
rates := NewRates(time.Time{}, nil)
rates := NewRates(nil)

// Execute:
rate, err := rates.GetRate("USD", "EUR")
Expand All @@ -232,7 +195,7 @@ func TestGetRate_EmptyRates(t *testing.T) {
func TestGetRate_NotValidISOCurrency(t *testing.T) {

// Setup:
rates := NewRates(time.Time{}, nil)
rates := NewRates(nil)

testCases := []struct {
from string
Expand Down
8 changes: 4 additions & 4 deletions endpoints/openrtb2/auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2774,7 +2774,7 @@ func (e *mockBidExchange) getAuctionCurrencyRates(customRates *openrtb_ext.ExtRe
if customRates == nil {
// The timestamp is required for the function signature, but is not used and its
// value has no significance in the tests
return currency.NewRates(time.Now(), e.pbsRates)
return currency.NewRates(e.pbsRates)
}

usePbsRates := true
Expand All @@ -2785,18 +2785,18 @@ func (e *mockBidExchange) getAuctionCurrencyRates(customRates *openrtb_ext.ExtRe
if !usePbsRates {
// The timestamp is required for the function signature, but is not used and its
// value has no significance in the tests
return currency.NewRates(time.Now(), customRates.ConversionRates)
return currency.NewRates(customRates.ConversionRates)
}

// Both PBS and custom rates can be used, check if ConversionRates is not empty
if len(customRates.ConversionRates) == 0 {
// Custom rates map is empty, use PBS rates only
return currency.NewRates(time.Now(), e.pbsRates)
return currency.NewRates(e.pbsRates)
}

// Return an AggregateConversions object that includes both custom and PBS currency rates but will
// prioritize custom rates over PBS rates whenever a currency rate is found in both
return currency.NewAggregateConversions(currency.NewRates(time.Time{}, customRates.ConversionRates), currency.NewRates(time.Now(), e.pbsRates))
return currency.NewAggregateConversions(currency.NewRates(customRates.ConversionRates), currency.NewRates(e.pbsRates))
}

// mockBidExchange is a well-behaved exchange that lists the bidders found in every bidRequest.Imp[i].Ext
Expand Down
Loading