Test parameter change cache miss #17346
Conversation
| { | ||
| name: 'eth_call', | ||
| blockParamIndex: 1, | ||
| numberOfParameters: 2, |
There was a problem hiding this comment.
I looked up each method in both the Ethereum JSON-RPC spec, and the Infura docs. I ensured the configured number of parameters matched the maximum shown in either docs.
They mostly matched already; I think there was a single method that had one less parameter on Infura because it didn't support some option, don't recall which one. And there were some methods exclusive to one or the other. But other than that they matched.
Tests have been added to ensure that our middleware will not return a cached response to a request with unique parameters. Each parameter supported by each method is tested independently. Closes #17003
Gudahtt
force-pushed
the
add-tests-to-ensure-cache-miss-when-param-changes
branch
from
ad3f591 to
21004bb
Compare
Builds ready [21004bb]
Page Load Metrics (1262 ± 96 ms)
Bundle size diffs
|
| }); | ||
| }); | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| describe('methods that assume there is no block param', () => { |
There was a problem hiding this comment.
I know this isn't the target of this PR so this is more of just an observation/question: this seems like an odd way to categorize the methods for testing?
There was a problem hiding this comment.
These groups are meant to reflect the caching strategies used in our middleware: either we ignore the method, or we cache it by the current block, or we cache it by the referenced block.
This "assume there is no block param" group is for methods that we cache by block, where the middleware doesn't bother looking for any block reference in the method parameters.
There was a problem hiding this comment.
Definitely lots of room for improvement with how these are organized. We can describe these categories better, and deduplicate the common behavior more effectively.
| @@ -452,6 +495,46 @@ export function testsForRpcMethodAssumingNoBlockParam( | |||
| }); | |||
| }); | |||
|
|
|||
| for (const paramIndex of [...Array(numberOfParameters).keys()]) { | |||
| it(`does not reuse the result of a previous request if parameter at index "${paramIndex}" differs`, async () => { | |||
There was a problem hiding this comment.
Took me a while to work through why this was testing what you claim its testing. But I see now! Looks good!
There was a problem hiding this comment.
Yeah... the param builder function didn't quite do what I wanted here, so I constructed the parameters inline in the test. I should probably find a way to make this more readable.
There was a problem hiding this comment.
Would you mind explaining which part you initially found confusing? That might help in finding a clearer way to write this.
There was a problem hiding this comment.
I don't think there's any issue with the description. I think it was just towards the end of my day and my brain wasn't working as well. So I guess this comment was more for me and not very generalizable 😅
| if (paramIndex === blockParamIndex) { | ||
| // testing changes in block param is covered under later tests | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I'm not understanding why we want to skip these cases for this case?
There was a problem hiding this comment.
These tests are basically meant to ensure that the middleware correctly recognizes any parameter changes, and doesn't return a cached response that was generated with different params. We want to check this for the block parameter as well, but, that one isn't so simple to make an arbitrary mutation to. The change will have other side-effects because the value is used by the caching middleware in other ways (it determines which block cache to use).
We do want to test the case where the block reference changes too, but, this test is within the block:
describe.each([
['given no block tag', undefined],
['given a block tag of "latest"', 'latest'],
])`
i.e. the block reference is always set to a static value in this test, that's what this category is for.
The case where the block reference is changed is covered by the test "does not reuse the result of a previous request if it was made with different arguments than this one", about ~1300 lines below here.
Tests have been added to ensure that our middleware will not return a cached response to a request with unique parameters. Each parameter supported by each method is tested independently.
Closes #17003
Manual Testing Steps
No functional changes.
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.