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.

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

Backfill tests for gas-util to raise coverage #736

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Mar 21, 2022

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.


Fixes #734.

@mcmire mcmire requested a review from a team as a code owner March 21, 2022 18:36
@mcmire mcmire force-pushed the backfill-tests-for-gas-util branch from 5e4c0ad to 93a8885 Compare March 23, 2022 03:36
@mcmire mcmire changed the base branch from backfill-types to main March 23, 2022 03:36
@mcmire mcmire mentioned this pull request Mar 23, 2022
2 tasks
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 mcmire force-pushed the backfill-tests-for-gas-util branch from 93a8885 to dfed08a Compare March 24, 2022 21:05
expect(normalizeGWEIDecimalNumbers(1234)).toBe('1234');
expect(normalizeGWEIDecimalNumbers(1000)).toBe('1000');
});
describe('fetchEthGasPriceEstimate', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This should include tests for the error case as well.

Copy link
Member

Choose a reason for hiding this comment

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

Also maybe for large numbers? To test whether the numbers supported are limited to those able to be represented by a JavaScript number.

Copy link
Contributor Author

@mcmire mcmire Mar 29, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

The await query call can throw, e.g. due to a network error.

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?

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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', () => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I can make this change.

});
});

describe('and the given priority fee reaches the suggested low priority fee threshold, but does reach the medium priority fee threshold', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This seemed more concise:

Suggested change
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".

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup I can do that!

@mcmire mcmire marked this pull request as draft December 20, 2022 17:52
@mcmire
Copy link
Contributor Author

mcmire commented Dec 20, 2022

Moving this back to draft until I address the feedback and fix the conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve test coverage for src/gas/gas-util.ts
2 participants