Skip to content

test: dapp-sdk unit tests#1992

Open
pawelstepien-da wants to merge 13 commits into
mainfrom
pawel/dapp-sdk-unit-tests
Open

test: dapp-sdk unit tests#1992
pawelstepien-da wants to merge 13 commits into
mainfrom
pawel/dapp-sdk-unit-tests

Conversation

@pawelstepien-da

Copy link
Copy Markdown
Contributor

Note:
dapp-sdk integration tests run together with units on yarn test / test:coverage, but only unit tests count to coverage.

Signed-off-by: Pawel Stepien <pawel.stepien@digitalasset.com>
Signed-off-by: Pawel Stepien <pawel.stepien@digitalasset.com>
Signed-off-by: Pawel Stepien <pawel.stepien@digitalasset.com>
Signed-off-by: Pawel Stepien <pawel.stepien@digitalasset.com>
Signed-off-by: Pawel Stepien <pawel.stepien@digitalasset.com>
Signed-off-by: Pawel Stepien <pawel.stepien@digitalasset.com>
Signed-off-by: Pawel Stepien <pawel.stepien@digitalasset.com>
Signed-off-by: Pawel Stepien <pawel.stepien@digitalasset.com>
Signed-off-by: Pawel Stepien <pawel.stepien@digitalasset.com>
Signed-off-by: Pawel Stepien <pawel.stepien@digitalasset.com>
@pawelstepien-da pawelstepien-da marked this pull request as ready for review June 12, 2026 13:55
@pawelstepien-da pawelstepien-da requested a review from a team as a code owner June 12, 2026 13:55
@pawelstepien-da pawelstepien-da linked an issue Jun 12, 2026 that may be closed by this pull request
alexmatson-da
alexmatson-da previously approved these changes Jun 15, 2026
Comment thread sdk/dapp-sdk/vitest.config.ts Outdated
@mateuszpiatkowski-da mateuszpiatkowski-da self-requested a review June 16, 2026 13:15

@mateuszpiatkowski-da mateuszpiatkowski-da left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be continued 😝

Comment thread sdk/dapp-sdk/vitest.config.ts Outdated
},
environment: 'node',
include: [],
// include: [],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is not needed anymore?

],
provider: 'v8',
reporter: ['text', 'html', 'lcov'],
thresholds: {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want to change the values here?

request: Mock<Provider<DappRpcTypes>['request']>
}

const connectedStatus = (): StatusEvent => ({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it need to be a function? I can see it only used in one place, so I'm not sure if we even need a const here tbh :D

description: 'Test extension',
icon: 'data:image/png;base64,abc',
})
expect(adapter.target).toBe('my-wallet')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're testing public props here, shouldn't we ensure that name, type, icon (and possibly target) are properly assigned to?

const adapter = new ExtensionAdapter()

expect(adapter.providerId).toBe('browser')
expect(adapter.getInfo().name).toBe('Browser Extension')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add the description property to test as well via getInfo()

},
}))

const kernelSession = (): StatusEvent => ({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it doesn't necessarily need to be a function if it's just an object which we read (function is ok though)

})

afterEach(() => {
vi.restoreAllMocks()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure we need it? We have the clearing of mocks in beforeEach hence the question

tx: { commandId: 'cmd-1', status: 'executed' },
})
mockController.signMessage.mockResolvedValue({ signature: 'sig' })
mockController.ledgerApi.mockResolvedValue({ ok: true })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want we can do sth like this:

Suggested change
mockController.ledgerApi.mockResolvedValue({ ok: true })
mockController.ledgerApi.mockResolvedValue({ ok: true })
adapter = new RemoteAdapter({
name: 'Gateway',
rpcUrl: RPC_URL
})

and just declare the var via let above

this way you can omit the need to create the adapter each time you want to use it.

'statusChanged',
listener
)
expect(mockDappAsyncProvider.emit).toHaveBeenCalledWith(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't want to check if listener has beed called too? I'm not into the business logic as much so I'm just not sure about it tho :p

expect(popup.close).toHaveBeenCalled()
})

describe('restore', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also add the test when discovery.walletType !== 'remote'

Signed-off-by: Pawel Stepien <pawel.stepien@digitalasset.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dapp-sdk unit tests

3 participants