chore: reduce diffs to GasFeeController.test.ts introduced by 2b1841c

[dbrans]

Explanation

Commit 2b1841c introduced unnecessary changes to GasFeeController.test.ts, complicating the process of rebasing and merging subsequent changes to this file.

To see the net diffs to GasFeeController.test.ts in this branch, you can compare that file to 2b1841c's parent commit, ca683e8:

git diff ca683e8...djb/gas-fee-tests packages/gas-fee-controller/src/GasFeeController.test.ts

References

Changelog

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[mcmire]

Hey @dbrans! Thanks for the ping on this. I left some comments. Curious to know more about the pain point you ran into and how you typically edit tests.

[mcmire]

If we do want to extract common setup to a function like this, then we should be careful that this matches the scenario we've describe and that all tests within the describe need this data to run. In this case, the describe says "when passed a networkClientId in options object". So the question is, networksMetadata need to be populated with all three of these networks in order to do that? Furthermore, does getIsEIP1559Compatible need to be true?

[dbrans]

Hi Elliot!

I understand from your comment that the resetMocks PR aims to make tests more explicit about what is being tested, which I fully support! Just want to make sure we’re keeping setup code concise and relevant to the tests.

For example, as you mentioned, much of the configuration options are not relevant to the tests they are contained in. I appreciate you inlining the test setup variables. I think a more careful, case-by-case approach might give better results. I'm happy to discuss how that might be done and to assist with that effort.

In terms of the difficulties with merging and rebasing, as an example, I'm trying to revert these changes to GasFeeController.test.ts and it requires some care.

[mcmire]

Well the original goal of the resetMocks PR was simply to make sure that mock functions were being properly reset for each test. I may have been overzealous in making the changes I did to GasFeeController but I wanted to make sure that we didn't run into issues with mock functions again. Using a function instead of a variable to store common options as you've done in this PR also prevents those issues, so I am happy with that solution. If your changes also make it easier to merge future changes then it makes sense to do what you've done here as well. We can always review the tests on a case-by-case basis and remove unnecessary setup later.

[mcmire]

The reason why I placed this kind of setup in the test itself is because this information is critical to the test (where critical means that if we didn't have this data here, then the behavior we're testing wouldn't make sense, because this is what creates the scenario that sets up the test). In my experience, it's better in the long run to place this kind of data in the body of the test itself (in the function passed to it) so that it is easier to comprehend the test without having to necessarily look elsewhere. I can understand that you might want to move this closer to the describe so that it is easier to cross-check with the description there, but if the describe ends up being long then you end up having to go back and forth between this and the test itself to mentally construct the data that is being built.

That said, this is dependent on my habits when it comes to writing tests. I find that I frequently need to edit tests in the middle of files for which I may have little context (either because I didn't write the tests or I did and I forgot how they work 😄 ) and the faster that I can do this the better. However, I realize everyone has different reading and writing styles and so I would definitely welcome your opinion here.

[mcmire]

LGTM!