-
Notifications
You must be signed in to change notification settings - Fork 970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Tests since MWA is no longer autoselected #1021
base: revert-1012
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how these changes "fix" the tests, but they do so in a way that breaks the intent of what we wanted to test in the first place, and also don't test the new behavior that we want to have with the changes this targets.
it('should not clear the stored wallet name', () => { | ||
expect(localStorage.removeItem).not.toHaveBeenCalled(); | ||
it('should clear the stored wallet name', () => { | ||
expect(localStorage.removeItem).toHaveBeenCalled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I think the original intent of the test is correct -- the wallet disconnecting should NOT deselect the wallet or remove it from LocalStorage, because then autoconnect will be broken on reload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this true? AFAICT when using wallet adapter, autoconnect is not supposed to reload the last used wallet if disconnect was called
renderTest({}); | ||
expect(ref.current?.getWalletContextState().wallet?.adapter.name).toBe(SolanaMobileWalletAdapterWalletName); | ||
expect(ref.current?.getWalletContextState().wallet?.adapter.name).toBe(undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead tests need to be changed/added such that
- If MWA (custom or default) is the only adapter, it should be selected by default.
- If it's not the only adapter, no adapter should be selected by default.
I think this satisfies the intent of the new code this is targeting.
beforeEach(async () => { | ||
errorThrown = new WalletError('o no'); | ||
onError = jest.fn(); | ||
renderTest({ onError }); | ||
await act(async () => { | ||
ref.current?.getWalletContextState().select(SolanaMobileWalletAdapterWalletName); | ||
await Promise.resolve(); // Flush all promises in effects after calling `select()`. | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't make sense to me. MWA shouldn't be deselected after each test, given the comment following this one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey sorry, not sure I totally follow – I don't think there's any selecting involved, the test is just to make sure that when the adapter emits an error, it expects onError
to be called. or was the point of the test something different?
No description provided.