Jl/caip multichain/test cleanups #26698
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
| invoker: 'test.com', | ||
| parentCapability: Caip25EndowmentPermissionName, | ||
| }); | ||
| }).toThrow(new Error('missing required caveat')); |
There was a problem hiding this comment.
Is there a todo to improve this error?
There was a problem hiding this comment.
i think we're going to have to just comb through all the changes in the feature branch to make sure we catch all these because not all of them have TODO comments
| expect(isChainIdSupportedBody).toContain('findNetworkClientIdByChainId'); | ||
| }); | ||
|
|
||
| it('throws if the input requiredScopes ScopesObject is not already validated and flattened', () => { |
There was a problem hiding this comment.
Should there be requiredScopes in the mocked value returned by the validateAndFlattenScopes mock for this test?
There was a problem hiding this comment.
see comment below #26698 (comment)
| }).toThrow(/Expected values to be strictly deep-equal/u); | ||
| }); | ||
|
|
||
| it('throws if the input optionalScopes ScopesObject is not already validated and flattened', () => { |
There was a problem hiding this comment.
Oh is the idea here that the validateAndFlattenScopes mock is returning an empty object for flattenedOptionalScopes because the optionalScopes were invalid?
There was a problem hiding this comment.
Maybe the wording of these tests could be a bit more clear? "Throws if the input does not match the output of validateAndFlattenScopes"?
There was a problem hiding this comment.
Oh is the idea here that the validateAndFlattenScopes mock is returning an empty object for flattenedOptionalScopes because the optionalScopes were invalid?
right. invalid OR just not flattened, but the point is that the input ScopesObject does not match
Maybe the wording of these tests could be a bit more clear? "Throws if the input does not match the output of validateAndFlattenScopes"?
I thought about this, but it seems too descriptive / describes the wrong thing? I mean it's not wrong, it just seems that our goal is to throw when the input ScopesObject is not validated and flattened. How we achieve that is an implementation detail.
There was a problem hiding this comment.
Yeah makes sense. I feel like not everyone feels the same way about naming/describing tests. Personally when the test name doesn't clearly/directly describe what's being tested I like to add inline comments to explain the discrepancy. But its not strictly necessary. And not everyone agrees with this thinking.
There was a problem hiding this comment.
Fair. It's unclear from the test setup why the mock return value of validateAndFlattenScopes must be changed. I've updated the scenario descriptions because of that bcd8ebc
| @@ -138,7 +133,7 @@ async function requestEthereumAccountsHandler( | |||
| properties: { | |||
| is_first_visit: isFirstVisit, | |||
| number_of_accounts: Object.keys(metamaskState.accounts).length, | |||
| number_of_accounts_connected: numberOfConnectedAccounts, | |||
| number_of_accounts_connected: accounts, | |||
There was a problem hiding this comment.
?
a) is accounts a number, as the property name seems to imply it should be?
b) how does this relate tot he other changes here?
There was a problem hiding this comment.
a) whoops... it should be a number, but it isn't currently. good catch
There was a problem hiding this comment.
b) getAccounts(), a hook we need for getting accounts ordered correctly, already gets the accounts from the permission. We don't need a separate call to getPermissionsForOrigin to also get the accounts (ordered potentially incorrectly, not that it matters). So i got rid of the getPermissionsForOrigin call and removed it from the hooks
Description
Test cleanup chores
Related issues
See: https://github.com/MetaMask/MetaMask-planning/issues/3046
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist