BCI-1426: Gas Price Subunits Changes #509
Conversation
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
| @@ -52,7 +59,14 @@ func (s staticDataSource) Evaluate(ctx context.Context, ds median.DataSource) er | |||
| return fmt.Errorf("failed to observe dataSource: %w", err) | |||
| } | |||
| if gotVal.Cmp(s.Value) != 0 { | |||
| return fmt.Errorf("expected Value %s but got %s", value, gotVal) | |||
| return fmt.Errorf("expected Value %s but got %s", s.Value, gotVal) | |||
There was a problem hiding this comment.
this was a prior typo where it was using the global variable value instead of the static data source's value
| @@ -93,6 +104,12 @@ func (s staticMedianFactoryServer) NewMedianFactory(ctx context.Context, provide | |||
| return nil, fmt.Errorf("NewMedianFactory: juelsPerFeeCoinDataSource does not equal a static test juels per fee coin data source implementation: %w", err) | |||
| } | |||
|
|
|||
| err = s.gasPriceSubunitsDataSource.Evaluate(ctx, gasPriceSubunitsDataSource) | |||
| // allow for testing 0 value with the same staticMedianFactoryServer (only defined once) | |||
| if err != nil && !strings.HasSuffix(err.Error(), "got 0") { | |||
There was a problem hiding this comment.
i'd have to rework a lot of logic if I'd want to create a staticMedianFactoryServer with 0 static data source value. Since this is just a test assertion i allow for 0 to come back as a valid value during the evaluation phase
There was a problem hiding this comment.
this is quite brittle. at minimium i would make a named error at the return site and use error.Is here rather than string matching
There was a problem hiding this comment.
sounds good, I can do that
| @@ -1,2 +1,3 @@ | |||
| golang 1.21.4 | |||
| golangci-lint 1.55.2 | |||
| mockery 2.38.0 | |||
There was a problem hiding this comment.
Is this functional?
We have a make command that installs directly: https://github.com/smartcontractkit/chainlink-common/blob/main/Makefile#L17-L18
There was a problem hiding this comment.
yeah it works , i use asdf locally so I wanted to install this. it's the same version as the makefile
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
| } | ||
|
|
||
| func (e *CompareError) GotZero() bool { | ||
| return e.Got.Uint64() == 0 |
There was a problem hiding this comment.
nit/ Could tighten this up a bit:
| return e.Got.Uint64() == 0 | |
| return e.Got.IsUint64() && e.Got.Uint64() == 0 |
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Failure
Since last time this change was worked on, the code base has changed significantly so I created a new branch.
Merge Order
Please see original PR description here for more information https://github.com/smartcontractkit/chainlink-common/pull/166/files