Skip to content

NetworkController: Normalize INFURA_NETWORKS array#1306

Merged
mcmire merged 1 commit intomainfrom
normalize-infura-networks-array
May 2, 2023
Merged

NetworkController: Normalize INFURA_NETWORKS array#1306
mcmire merged 1 commit intomainfrom
normalize-infura-networks-array

Conversation

@mcmire
Copy link
Contributor

@mcmire mcmire commented May 2, 2023

Description

The unit tests for NetworkController use an object of networks which is used in order to exercise code that creates an Infura provider. The NetworkController unit tests on the extension side, however, use an array instead of an object. This commit makes the two sides more consistent.

Changes

N/A

References

Reduces the scope of #1116.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

The unit tests for NetworkController use an object of networks which is
used in order to exercise code that creates an Infura provider. The
NetworkController unit tests on the extension side, however, use an
array instead of an object. This commit makes the two sides more
consistent.
@mcmire mcmire requested a review from a team as a code owner May 2, 2023 20:46
chainId: NetworksChainId.mainnet,
ticker: NetworksTicker.mainnet,
blockExplorerUrl: `https://etherscan.io`,
chainId: '1',
Copy link
Member

@Gudahtt Gudahtt May 2, 2023

Choose a reason for hiding this comment

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

Nit: While the constants we're using in this package aren't great, using a constant here rather than hard-coding a value did seem like a good idea. Have you considered restoring the use of NetworksChainId and NetworksTicker?

Copy link
Contributor Author

@mcmire mcmire May 2, 2023

Choose a reason for hiding this comment

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

Yes, so my feeling in general is that tests shouldn't share constants with implementation code if they don't have to. The thought here is that if the value in the constant is wrong, then you won't know it's wrong just by running the tests. It sort of feels like stubbing the thing under test to return a value, calling that thing, and then asserting that the expected and actual values match. But if you repeat the value in the test, then it gives you another opportunity to confirm that it's right. I thought it was okay to share NetworkType because it's a type as much as it is a value, so if it's wrong, then our whole interface is wrong. But we don't use NetworksChainId and NetworksTicker as types, so I chose to repeat them. Maybe this is not a huge deal, and it certainly is convenient just to reuse values. I could be convinced otherwise, I suppose. But that's where that's coming from.

Copy link
Member

Choose a reason for hiding this comment

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

Ah OK. I'm generally a fan of using constants in tests. It can make the test suite much friendlier, easier to modfiy when code changes and easier to read.

The type of errors this would protect against can happen in the reverse direction as well (e.g. a test appears to pass only because the hard-coded constant is wrong), so it's not necessarily safer to hard-code constants either.

But we can discuss some other time, this is good for now. These constants you've replaced here were confusing anyway; this is a readability improvement for that reason alone.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcmire mcmire merged commit 6ded2a3 into main May 2, 2023
@mcmire mcmire deleted the normalize-infura-networks-array branch May 2, 2023 21:21
Gudahtt added a commit that referenced this pull request May 3, 2023
…-rebased

* origin/main:
  docs: update controller-utils/isValidHexAddress to match reality (#1308)
  keyring-controller: validate from-address in signTypedMessage (#1293)
  NetworkController: Fix chain IDs in tests (#1307)
  NetworkController: Normalize INFURA_NETWORKS array (#1306)
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The unit tests for NetworkController use an object of networks which is
used in order to exercise code that creates an Infura provider. The
NetworkController unit tests on the extension side, however, use an
array instead of an object. This commit makes the two sides more
consistent.
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The unit tests for NetworkController use an object of networks which is
used in order to exercise code that creates an Infura provider. The
NetworkController unit tests on the extension side, however, use an
array instead of an object. This commit makes the two sides more
consistent.
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.

2 participants