Convert NetworkController unit tests to TypeScript #18476
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We want to convert NetworkController to TypeScript in order to be able to compare differences in the controller between in this repo and the core repo. To do this, however, we need to convert the dependencies of the controller to TypeScript. As a part of this effort, this commit converts `shared/constants/metametrics` to TypeScript. Note that simple objects have been largely replaced with enums. There are some cases where I even split up some of these objects into multiple enums.
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
- There are several categories of event sources. Rearrange the constant names to accentuate this. For instance, `MetaMetricsEventNetworkSource` would seem (when reading this as English) to indicate that we have a "network source" that is related to a MetaMetrics event. But really, it's a MetaMetrics event source that we've classified under "network". - An external link type is just a link type (according to how it's used). There is no other kind of link type. Rename this constant accordingly. - We don't include "properties" in the name of the types. For instance a key type is passed as a property, but we just call the type `MetaMetricsEventKeyType`. Rename `MetaMetricsEventUiCustomizationPropertyValue` accordingly.
Converting this controller to TypeScript furthers the goal of getting this whole codebase converted, of course, but it also helps in comparing the differences between this version of the NetworkController and the version in the `core` repo more easily, which will ultimately help us in coalescing the two implementations.
…controller-tests-to-ts
…controller-tests-to-ts
Builds ready [b5402a6]
Page Load Metrics (1780 ± 83 ms)
Bundle size diffs
|
Gudahtt
reviewed
Apr 12, 2023
Gudahtt
reviewed
Apr 12, 2023
| }): Promise<E['payload'][]> { | ||
| const promiseForEventPayloads = new Promise<E['payload'][]>( | ||
| (resolve, reject) => { | ||
| // Everything depends on `eventListener` circularly, so we need to declare |
There was a problem hiding this comment.
Nit: This could be simplifed by using a function declaration for end, stopTimer, and resetTimer. Function declarations make circular references easier to handle due to function hoisting; the declaration is hoisted to the top of the scope, so they can be referenced before the declaration.
That lets use const when defining the event listener, and saves us from needing this comment to explain the awkward order of operations.
There was a problem hiding this comment.
Hmm you're right! I notice the same thing happens in waitForStateChanges but I'll fix that later. But I can fix this one now: 45f001b
Gudahtt
reviewed
Apr 12, 2023
Builds ready [f27584c]
Page Load Metrics (1693 ± 69 ms)
Bundle size diffs
|
Gudahtt
previously approved these changes
Apr 12, 2023
Gudahtt
approved these changes
Apr 12, 2023
digiwand
reviewed
Apr 12, 2023
Comment on lines
+518
to
+519
| * @param args.infuraProjectId - TODO. | ||
| * @param args.infuraNetwork - TODO. |
There was a problem hiding this comment.
what are these TODO comments for? 👀
There was a problem hiding this comment.
Oops, good catch, thanks. Fortunately I'm going to update this test file again in another PR very soon so I will address them then.
digiwand
approved these changes
Apr 12, 2023
Builds ready [ace10d6]
Page Load Metrics (1553 ± 31 ms)
Bundle size diffs
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
This helps us more easily compare the unit tests for NetworkController in this repo and the NetworkController in the
corerepo.Closes #18424.
Manual Testing Steps
No functional changes. All tests should pass.
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.