Improve unit tests #219
Improve unit tests #219
Conversation
The mocks used in the external provider unit tests have been rewritten to allow sending and inspecting messages in either direction. They also no longer rely upon mocking out internal methods. The `getInitializedInpageProvider` helper function in the inpage provider tests has been updated with method callbacks, to allow tests to setup replies for specific messages. This will be used in a future PR. One additional test was added to ensure the `isMetaMask` property is set on the inpage provider.
Gudahtt
force-pushed
the
improve-unit-tests
branch
from
c06fd76
to
afeb53a
Compare
| import { createExternalExtensionProvider } from './createExternalExtensionProvider'; | ||
|
|
||
| describe('createExternalExtensionProvider', () => { | ||
| beforeEach(() => { | ||
| it('can be called and not throw', () => { | ||
| (global.chrome.runtime.connect as any).mockImplementation(() => { |
There was a problem hiding this comment.
Technically I didn't need to move this from the beforeEach into the individual tests here, but it's needed in the next PR because it conflicts with some of the setup in later tests.
src/MetaMaskInpageProvider.test.ts
Outdated
| if (!isInitialized) { | ||
| if ( | ||
| name === 'metamask-provider' && | ||
| data.method === 'metamask_getProviderState' |
There was a problem hiding this comment.
I notice that with MockDuplexStream, we didn't handle checking for metamask_getProviderState. I think I might know the answer to this, but why do we have to do this now?
There was a problem hiding this comment.
Good question! Previously the initial provider state was set by mocking out the request function and returning the initial state directly, bypassing the entire RPC layer. metamask_getProviderState was never really called, and any code past that request call was not run at all.
That mock was removed, and instead now we're using a mock connection instead. As such, now the entire initialization process is being tested, rather than just up to the .request invocation.
This means our mock has to behave roughly like a real connection with the wallet would though, so it's a bit more complex. This mock is setup to handle the initialization call, then pass along all other calls to the configured callbacks.
There was a problem hiding this comment.
This conversation had me re-thinking this initialization code, and in retrospect I think this protection is unnecessary and potentially harmful. This logic was simplified here, with the explanation for why in the commit message: 8eee8b8
The initialization step of the mock provider has been simplified. The initialization event is awaited directly rather than using the more complicated decomposed promise pattern.
The return type of `getInitializedProvider` is now an object rather than a tuple. This was done to improve readability. Now it's more clear exactly what each property is without close inspection. The return type has been improved as well. Beforehand it was inferred by TypeScript, but now it is explicitly declared as a type and documented.
A protection measure was setup in the initialized provider helper function to prevent it from responding to messages before the initialization was complete This has been removed. It was originally added during debugging, to rule out before- initialization messages as an explanation for a bug that was later fixed I left it in because I thought that it might better match expectations if messages before initialization were ignored. But in retrospect, I think this would be both unnecessary and harmful. It's unnecessary because for most tests, no messages would be sent until after initialization anyway. And it's potentially harmful because this doesn't represent how the provider actually behaves. This could disguise a mistake in a test, or it could get in the way of testing strange edge cases surrounding initialization.
The mocks used in the external provider unit tests have been rewritten to allow sending and inspecting messages in either direction. They also no longer rely upon mocking out internal methods.
The
getInitializedInpageProviderhelper function in the inpage provider tests has been updated with method callbacks, to allow tests to setup replies for specific messages. This will be used in a future PR.One additional test was added to ensure the
isMetaMaskproperty is set on the inpage provider.