Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: revert-1012
Choose a base branch
from
+38 −22
Open

Fix Tests since MWA is no longer autoselected #1021

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ea09cb
update test
alex-fung Sep 5, 2024
5df03df
update test names
alex-fung Sep 5, 2024
c28ccdd
update tests
alex-fung Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions packages/core/react/src/__tests__/WalletProviderMobile-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,9 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
adapters.push(customAdapter);
jest.clearAllMocks();
});
it('loads the custom mobile wallet adapter into state as the default', () => {
it('does not load the custom mobile wallet adapter into state as the default', () => {
renderTest({});
expect(ref.current?.getWalletContextState().wallet?.adapter).toBe(customAdapter);
expect(ref.current?.getWalletContextState().wallet?.adapter).toBe(undefined);
});
it('does not construct any further mobile wallet adapters', () => {
renderTest({});
Expand All @@ -232,9 +232,9 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
beforeEach(() => {
localStorage.removeItem(WALLET_NAME_CACHE_KEY);
});
it('loads the mobile wallet adapter into state as the default', () => {
it('does not load the mobile wallet adapter into state as the default', () => {
renderTest({});
expect(ref.current?.getWalletContextState().wallet?.adapter.name).toBe(SolanaMobileWalletAdapterWalletName);
expect(ref.current?.getWalletContextState().wallet?.adapter.name).toBe(undefined);
Copy link
Collaborator

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.

});
it('loads no public key into state', () => {
renderTest({});
Expand Down Expand Up @@ -326,10 +326,14 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
describe('onError', () => {
let onError: jest.Mock;
let errorThrown: WalletError;
beforeEach(() => {
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()`.
});
Comment on lines +341 to +348
Copy link
Collaborator

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)

Copy link
Contributor Author

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?

});
describe('when the wallet emits an error', () => {
let adapter: Adapter;
Expand Down Expand Up @@ -452,8 +456,8 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
mobileWalletAdapter.disconnect();
});
});
it('should not clear the stored wallet name', () => {
expect(localStorage.removeItem).not.toHaveBeenCalled();
it('should clear the stored wallet name', () => {
expect(localStorage.removeItem).toHaveBeenCalled();
});
});
describe('when window beforeunload event fires', () => {
Expand All @@ -470,8 +474,8 @@ describe('WalletProvider when the environment is `MOBILE_WEB`', () => {
mobileWalletAdapter.disconnect();
});
});
it('should not clear the stored wallet name', () => {
expect(localStorage.removeItem).not.toHaveBeenCalled();
it('should clear the stored wallet name', () => {
expect(localStorage.removeItem).toHaveBeenCalled();
Copy link
Collaborator

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.

Copy link
Contributor Author

@alex-fung alex-fung Sep 5, 2024 •
edited
Loading

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

});
it('should clear out the state', () => {
expect(ref.current?.getWalletContextState()).toMatchObject({
Expand Down