Uh oh!
There was an error while loading. Please reload this page.
test: cleanup/fix SelectedNetworkController specs - #4409
Conversation
| describe('networkController:stateChange', () => { | ||
| describe('when useRequestQueuePreference is false', () => { | ||
| describe('when a networkClient is deleted from the network controller state', () => { |
There was a problem hiding this comment.
I did this originally, but while you're in here doing cleanup: I've received feedback elsewhere that having this many nested describes can be confusing. I think in this case the last one could be removed since it wraps only one test and you could rephrase the it block to it('does not update state when a networkClient is deleted from the network controller state')
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| describe('PermissionController:stateChange', () => { | ||
| describe('on permission add', () => { | ||
| describe('when useRequestQueuePreference is true', () => { |
There was a problem hiding this comment.
Same point as above about nesting multiple describe blocks when the innermost one only wraps one test. I introduced similar patterns. I'm not personally too troubled about it but wonder what others feels about this @shanejonas@mcmire@Gudahtt ?
There was a problem hiding this comment.
I agree with being cautious about nesting describe blocks too much. After 3 or 4 is where I start to get confused as well (looking back, I made this mistake while writing NetworkController tests and I would love to clean those up at some point).
There was a problem hiding this comment.
specs have been flattened where possible
| describe('Constructor checks for domains in permissions', () => { | ||
| describe('when useRequestQueuePreference is true', () => { | ||
| it('should set networkClientId for domains not already in state', async () => { |
There was a problem hiding this comment.
isn't this test still valid?
There was a problem hiding this comment.
these were moved into the existing constructor scenario. See https://github.com/MetaMask/core/pull/4409/files#diff-a9e0027991e57fd3175c02854cb99119a961b43dedc6758e0a117ebe6321e428R230
…roller.test.ts Co-authored-by: Alex Donesky <adonesky@gmail.com>
…roller.test.ts Co-authored-by: Alex Donesky <adonesky@gmail.com>
…roller.test.ts Co-authored-by: Alex Donesky <adonesky@gmail.com>
…roller.test.ts Co-authored-by: Alex Donesky <adonesky@gmail.com>
…roller.test.ts Co-authored-by: Alex Donesky <adonesky@gmail.com>
Cleans up / fixes some `SelectedNetworkController` specs Fixes: MetaMask/MetaMask-planning#2626 No consumer facing changes - [x] 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 --------- Co-authored-by: Alex Donesky <adonesky@gmail.com>
Explanation
Cleans up / fixes some
SelectedNetworkControllerspecsReferences
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2626
Changelog
No consumer facing changes
Checklist