Backfill tests for gas-util to raise coverage
#736
Conversation
mcmire
force-pushed
the
backfill-tests-for-gas-util
branch
from
5e4c0ad to
93a8885
Compare
Recent changes to `gas-util` lowered test coverage, forcing exclusion from the Jest coverage report. These changes allow us to include this file again by raising coverage.
mcmire
force-pushed
the
backfill-tests-for-gas-util
branch
from
93a8885 to
dfed08a
Compare
| expect(normalizeGWEIDecimalNumbers(1234)).toBe('1234'); | ||
| expect(normalizeGWEIDecimalNumbers(1000)).toBe('1000'); | ||
| }); | ||
| describe('fetchEthGasPriceEstimate', () => { |
There was a problem hiding this comment.
This should include tests for the error case as well.
There was a problem hiding this comment.
Also maybe for large numbers? To test whether the numbers supported are limited to those able to be represented by a JavaScript number.
There was a problem hiding this comment.
What error case were you thinking of?
For numbers, were you thinking what if the gasPrice is some big number? If so, weiHexToGweiDec should be able to handle big numbers correctly, so can we trust that it's doing the right thing? If we are lacking tests around weiHexToGweiDec, then that's understandable, but maybe we double-check number support in those tests?
There was a problem hiding this comment.
The await query call can throw, e.g. due to a network error.
For numbers, were you thinking what if the
gasPriceis some big number? If so,weiHexToGweiDecshould be able to handle big numbers correctly, so can we trust that it's doing the right thing? If we are lacking tests aroundweiHexToGweiDec, then that's understandable, but maybe we double-check number support in those tests?
Right, but the unit tests should establish what the function is meant to do, irrespective of the current implementation. If/when we decide to replace BN for example, we'll want to ensure we don't unknowingly introduce a change here.
There was a problem hiding this comment.
Okay, I can add some overlapping test coverage.
I understand that the thing we know the least about, and can hence trust the least, is ethQuery.gasPrice() / eth_gasPrice. Do you think it is worth testing what happens if gasPrice is null, undefined, empty string, something that is not a hex string, etc.? I'd like to think we are able to make some assumptions here.
There was a problem hiding this comment.
Hmm. Good question. According to the wiki, it should always return a "QUANTITY", i.e. a string hex number. So those should all be impossible. But better if we can avoid trusting third party APIs, especially when the user can set them to a custom node that doesn't follow these rules.
This is a good example of a case where extracting an API interaction into a separate module might be simpler. So that in cases like this, we can trust that the return type is what we expect. Then in our API module we have validation and throw if anything is wrong.
So... the point being, you raise a great point and we should handle that but perhaps it can be done elsewhere, in a dedicated API module or built in to the query function we're calling EthQuery with here, something like that.
| '1.000000017', | ||
| ); | ||
| }); | ||
| describe('if the given priority fee is less than the given max fee minus the latest base fee', () => { |
There was a problem hiding this comment.
Nit: This was easier for me to understand when I rephrased it to be "If the priority + base fee is (less than/equal to/exceeds) the max fee". The "A < B - C" was a lot to parse.
I wish there was a better way to explain this than that even 🤔 But I can't think of anything better.
There was a problem hiding this comment.
Makes sense, I can make this change.
src/gas/gas-util.test.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| describe('and the given priority fee reaches the suggested low priority fee threshold, but does reach the medium priority fee threshold', () => { |
There was a problem hiding this comment.
Nit: This seemed more concise:
| describe('and the given priority fee reaches the suggested low priority fee threshold, but does reach the medium priority fee threshold', () => { | |
| describe('and the given priority fee is equal to the suggested low priority fee', () => { |
and the same for the others, except replace "equal to" with "exceeds", or "is between the suggested __ and __ priority fee".
There was a problem hiding this comment.
Though, this wording does remind me, is it possible for the suggested low and medium priority fee to be identical? Maybe we should test that case as well.
There was a problem hiding this comment.
It is possible. That's the reason I used "threshold" in the names of the tests. But I see how that would be confusing. I can revisit this.
There was a problem hiding this comment.
Oh, regarding low and medium being equal, what I meant was that this circumstance was not covered by any existing test. I didn't think the current descriptions implied it was impossible or anything, just that we could add a test case to cover this. Or multiple I guess (one for low === medium, one for medium === high, and one for all three being equal).
|
Moving this back to draft until I address the feedback and fix the conflicts. |
Recent changes to
gas-utillowered test coverage, forcing exclusionfrom the Jest coverage report. These changes allow us to include this
file again by raising coverage.
Fixes #734.