test: deeplink connect native #14667

karliatto · 2024-10-01T21:23:07Z

Description

Add new native e2e android test suite-native/app/e2e/tests/deeplinkPopup.test.ts where we call a @trezor/connect-mobile and get response in callback of a test server to check that we are getting the right response.

Related Issue

Related #14744

suite-native/app/e2e/tests/deeplinkPopup.test.ts

karliatto · 2024-10-21T09:07:53Z

I think this is ready.

mroz22

looks good to me. just left 2 useless comments, feel free to ignore.

maybe just ask mobile guys if they want to have a look too? Or is @HajekOndrej interested here?

mroz22 · 2024-10-21T10:17:54Z

suite-native/app/e2e/tests/deeplinkPopup.test.ts

+import { onCoinEnablingInit } from '../pageObjects/coinEnablingActions';
+import { onHome } from '../pageObjects/homeActions';
+
+const SERVER_PORT = 8080;


Is there any chance that I would be running this locally while having connect-explrorer dev server active? I think that we are using this port somewhere already. clash is probably unlikely however.

As far as I know connect-explorer is running on port 8088.

right, I saw it here https://github.com/trezor/trezor-suite/blob/develop/docs/features/coinjoin.md#L15

mroz22 · 2024-10-21T10:20:03Z

suite-native/app/e2e/tests/deeplinkPopup.test.ts

+        server.listen(SERVER_PORT, 'localhost', () => {
+            // eslint-disable-next-line no-console
+            console.info(`Server running at ${SERVER_URL}`);
+        });


nitpick: execution doesn't wait for server to be already listening. it could lead to a racecondition that is very unlikely to happen

That is a very good point. This should love that 9b9116f

suite-native/app/e2e/tests/deeplinkPopup.test.ts

PeKne

Great idea to write an automated test for this. This will save us a lot of time debugging later if someone breaks the connect popup integration. 👏

.github/workflows/test-suite-native-e2e-android.yml

packages/trezor-user-env-link/src/api.ts

suite-native/app/e2e/tests/deeplinkPopup.test.ts

PeKne · 2024-10-22T05:41:42Z

suite-native/app/e2e/tests/deeplinkPopup.test.ts

+            success: true,
+        };
+
+        await new Promise((resolve, reject) => {


why is the Promise object needed here? What about simplifying it to something like:

Suggested change

await new Promise((resolve, reject) => {

if (JSON.stringify(response) !== JSON.stringify(expectedResponse)) {

new Error('Result does not match expected.')

}

Done simpler 72c023f#diff-0cba7b27a14d30c9b8c7c5b3ac0c073b9e94a549ac4c8aeb74012a0676faba91R123

This, unfortunately, does not work I tried to put a dummy value to the expectedResponse e.g. {success: false}, and the test still passed. It is because if the values do not match, an error object is created, but it is not thrown.

To ensure the values are compared correctly, you must use the jest assert function: expect (response).toEqual(expectedResponse). I am aware that I recommended you to use the Error object in the previous comment, but I was rather focusing on removing the Promise object.

Sure, let's throw 407c5d0

expect does not work as expected, so I would keep the current approach but throwing the error, I tested with success and fail and it worked.

Strange, the expect worked for me locally. I like the expect keyword more because it prints out the difference between the two compared values in case of failure. Never mind, this works as well :).

karliatto · 2024-10-22T15:26:39Z

@PeKne I responded to all your request and tests are still passing. Let me know!

PeKne · 2024-10-23T06:00:15Z

Also one more thing. Now the test is working only for Android platform, but our tests are written for both Android and iOS. So it will always fail on Apple devices. For this reason, some platform condition should be introduced. ChatGPT offered me this solution (I tested it and it works):

...

// Define a function to conditionally skip or run the test suite
function conditionalDescribe(skipCondition: boolean, title: string, fn: jest.EmptyFunction) {
    if (skipCondition) {
        describe.skip(title, fn);
    } else {
        describe(title, fn);
    }
}

conditionalDescribe(device.getPlatform() !== 'android', 'Deeplink connect popup.', () => {
...

IMO it would make sense to put the conditionalDescribe to test utils in suite-common. It might be helpful later to someone else.

mroz22 · 2024-10-23T08:04:43Z

IMO it would make sense to put the conditionalDescribe to test utils in suite-common. It might be helpful later to someone else.

this should be solved even more globally. we already use a custom conditionalTest in connect for example. cc @HajekOndrej

martykan · 2024-10-23T08:11:47Z

I believe we can test iOS as well, it should work on both with the emulator.

martykan · 2024-10-23T12:43:54Z

So I attempted to adjust the test for iOS, however there are some issues during onboarding and coin enabling, which block the progress of the test:

The app is busy with the following tasks:
• There are 1 work items pending on the dispatch queue: "Main Queue (<OS_dispatch_queue_main: com.apple.main-thread>)".
• 1 enqueued native timers:
- Timer #1:
    + Fire date: none.
    + Time until fire: 0.000.
    + Repeat interval: 0.
    + Is recurring: YES.
• Run loop "Main Run Loop" is awake.

So overall the iOS E2E tests first need changes to support device connection.

PeKne · 2024-10-23T12:53:07Z

@martykan We do not want to make iOS tests behave like Android devices. At the moment, the flows and UI are different between iOS and Android. So, it is not intended to make the tests do the same thing on both platforms. Thus, device connection should not be allowed for iOS e2e tests now until we introduce a Trezor device supporting Bluetooth.

martykan · 2024-10-23T13:39:31Z

Ok, understandable, but currently all the iOS tests don't match the real behavior of the app.
When I build and run ios.sim.release tests, it's actually using the Debug variant, which supports connecting device and the flow is the same as Android.
Or maybe my setup is wrong?

PeKne · 2024-10-23T13:48:09Z

@martykan Good point. We have not used iOS tests in CI until now, so it is highly possible that the iOS test does not behave as it should. However, soon, there will also be an iOS test run in CI, so it will need to be fixed.

PeKne

Good job 👏

karliatto force-pushed the test/deeplink-native branch from d24e263 to 4a03590 Compare October 1, 2024 21:34

mroz22 reviewed Oct 2, 2024

View reviewed changes

suite-native/app/e2e/tests/deeplinkPopup.test.ts Outdated Show resolved Hide resolved

karliatto force-pushed the test/deeplink-native branch 5 times, most recently from f404274 to 0373f31 Compare October 7, 2024 09:45

karliatto mentioned this pull request Oct 7, 2024

@trezor/conenct-deeplink to production #14744

Open

5 tasks

karliatto force-pushed the test/deeplink-native branch 13 times, most recently from cfc67fb to b94c7dc Compare October 21, 2024 08:21

karliatto marked this pull request as ready for review October 21, 2024 09:06

karliatto requested review from a team and vdovhanych as code owners October 21, 2024 09:06

karliatto changed the title ~~wip: test: deeplink connect native~~ test: deeplink connect native Oct 21, 2024

mroz22 reviewed Oct 21, 2024

View reviewed changes

suite-native/app/e2e/tests/deeplinkPopup.test.ts Outdated Show resolved Hide resolved

PeKne requested changes Oct 22, 2024

View reviewed changes

PeKne approved these changes Oct 24, 2024

View reviewed changes

karliatto added 2 commits October 24, 2024 08:35

chore(trezor-user-env-link): add new mnemonic

97f8a6d

test(module-connect-popup): add testID to connect popup screen

1d63219

karliatto force-pushed the test/deeplink-native branch from 9bce10c to 571e8da Compare October 24, 2024 06:35

karliatto added 3 commits October 24, 2024 08:43

test(suite-native): new deeplinkPopup test

2f3ecbe

feat(test-utils): conditionalDescribe

b49f748

fix(suite-native): add conditionalDescribe to deeplink test

e8feabe

karliatto force-pushed the test/deeplink-native branch from 571e8da to e8feabe Compare October 24, 2024 06:43

karliatto merged commit 84453a6 into develop Oct 24, 2024
88 checks passed

karliatto deleted the test/deeplink-native branch October 24, 2024 07:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: deeplink connect native #14667

test: deeplink connect native #14667

karliatto commented Oct 1, 2024 •

edited

Loading

karliatto commented Oct 21, 2024

mroz22 left a comment

mroz22 Oct 21, 2024

karliatto Oct 21, 2024

mroz22 Oct 21, 2024

mroz22 Oct 21, 2024

karliatto Oct 21, 2024

PeKne left a comment

PeKne Oct 22, 2024 •

edited

Loading

karliatto Oct 22, 2024 •

edited

Loading

PeKne Oct 23, 2024 •

edited

Loading

karliatto Oct 24, 2024

PeKne Oct 24, 2024

karliatto commented Oct 22, 2024

PeKne commented Oct 23, 2024 •

edited

Loading

mroz22 commented Oct 23, 2024

martykan commented Oct 23, 2024

martykan commented Oct 23, 2024

PeKne commented Oct 23, 2024

martykan commented Oct 23, 2024

PeKne commented Oct 23, 2024 •

edited

Loading

PeKne left a comment

-        await new Promise((resolve, reject) => {
+        if (JSON.stringify(response) !== JSON.stringify(expectedResponse)) {
+                new Error('Result does not match expected.')
+            }

test: deeplink connect native #14667

test: deeplink connect native #14667

Conversation

karliatto commented Oct 1, 2024 • edited Loading

Description

Related Issue

karliatto commented Oct 21, 2024

mroz22 left a comment

Choose a reason for hiding this comment

mroz22 Oct 21, 2024

Choose a reason for hiding this comment

karliatto Oct 21, 2024

Choose a reason for hiding this comment

mroz22 Oct 21, 2024

Choose a reason for hiding this comment

mroz22 Oct 21, 2024

Choose a reason for hiding this comment

karliatto Oct 21, 2024

Choose a reason for hiding this comment

PeKne left a comment

Choose a reason for hiding this comment

PeKne Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

karliatto Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

PeKne Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

karliatto Oct 24, 2024

Choose a reason for hiding this comment

PeKne Oct 24, 2024

Choose a reason for hiding this comment

karliatto commented Oct 22, 2024

PeKne commented Oct 23, 2024 • edited Loading

mroz22 commented Oct 23, 2024

martykan commented Oct 23, 2024

martykan commented Oct 23, 2024

PeKne commented Oct 23, 2024

martykan commented Oct 23, 2024

PeKne commented Oct 23, 2024 • edited Loading

PeKne left a comment

Choose a reason for hiding this comment

karliatto commented Oct 1, 2024 •

edited

Loading

PeKne Oct 22, 2024 •

edited

Loading

karliatto Oct 22, 2024 •

edited

Loading

PeKne Oct 23, 2024 •

edited

Loading

PeKne commented Oct 23, 2024 •

edited

Loading

PeKne commented Oct 23, 2024 •

edited

Loading