Grdddj/replacing old firmwares in e2e tests

[grdddj]

Putting latest version of TT emulator (T1 may be done as well) - 2-master wherever possible into e2e tests
Notes/issues/ideas:

• created new clickEmu Cypress task, even though it is not being used currently (resolved in firmware issue) - in that test, we were sending the "all" mnemonic 13 times instead of 12 times, so it meant the pressYes was not working after that
• changed the clicked firmware button - continue-button instead of skip-button - as we are always using the latest version
• lot of new emulator changes were adding confirmation on device, a lot of times it was two confirmation in a row (we can add a Cypress task pressYesMultipleTimes(n))
• I have issues with some passphrase tests, where updating emulator to the latest one causes emulator initialization error: TrezorFailure - DataError: Passphrase is not enabled. Tried to play with arguments to setupEmu and applySettings, but could not make it work. Currently, these tests have 2.3.1 version with TODO above them to point out this issue
• most probably, the current commits would need to be squashed into one or two, to form a good PR, they are not good now

[grdddj]

Commit bd9d24c:

• in firmware update test, we should not have the latest emulator version, downgrading it to 2.4.1

Currently failing tests with new 2-master emulator (apart from the "passphrase" ones above):

• recovery/t2-dry-run-persistence.test.ts (is unstable even with 2.3.1)
  • when checking the backup, we have some issues (below)
  • however, when manually clicking Close and trying again, it works fine
  • 

[grdddj]

Commit 6a1eb08:

• upgrading emulator versions even for T1 (to 1-master)
• (did not test it locally, will wait for CI to show any possible issues)

Consideration:

• should we really test Suite with master builds instead of the latest released versions (2.4.1 in this case)?
  • it could mean firmware changes can break Suite tests, which is not very good
  • on the other hand, it will force us to always be up-to-date between Suite and firmware

[grdddj]

Commit b47be09:

• resolving the unstable T1 test

[tsusanka]

should we really test Suite with master builds instead of the latest released versions (2.4.1 in this case)?
- it could mean firmware changes can break Suite tests, which is not very good
- on the other hand, it will force us to always be up-to-date between Suite and firmware

This is something I wanted to raise myself. I have a preference to indeed have it against master. My line of thoughts:

1. We do have a history of small mess-ups where we have found out some Firmware <-> Suite inconsistencies very late in the process. This way we will know ASAP.
2. It is true that making a mistake in FW's master will lead to Suite e2e tests fail which is very unfortunate. On the other hand we have quite a comprehensive test suite on FW as well and we of course merge only after it passes. So this indeed should happen sporadically.

In a nutshell I suggest to indeed try to test against FW master and see where it gets us. If it would turn out problematic, we can always revert. I would like this be ACKed by @mroz22 @matejcik and @matejkriz though. What do you guys think?

[matejcik]

I'm actually somewhat against. I don't mind too much because this is not my CI :) but pulling from master means that Suite CI results can change randomly in the middle of the day, between runs of the same pipeline.

Counter-proposal: run Suite e2e against tagged version of firmware, bump tag automatically in a nightly job.
(by which I mean, store commit id of current firmware master somewhere, use this commit id instead of master)

[tsusanka]

Sorry for not being clear, but that is what we are basically suggesting. FW's master is added into trezor-user-env docker image using a scheduled nightly job, so it contains master up to 1 day old, it is does not fetch master directly.

There is one tiny exception and that is when master is changed in trezor-user-env, that means some development in trezor-user-env occured (aka PR is merged) which triggers the image to be rebuilt. But that can be avoided easily.

[tsusanka]

...or we could bundle the newest master weekly. Like over the weekend - as a compromise. We could run it e.g. on Sunday and also restart the Suite CI. If it would fail we could inform on Slack. This way we would know on Monday that something is wrong right away.

[matejcik]

Oh ok. in that case +1 from me.
It would also be nice to have the reverse for firmware CI.

[tsusanka]

I have issues with some passphrase tests, where updating emulator to the latest one causes emulator initialization error: TrezorFailure - DataError: Passphrase is not enabled. Tried to play with arguments to setupEmu and applySettings, but could not make it work. Currently, these tests have 2.3.1 version with TODO above them to point out this issue

Is the applySettings command working properly? You can find out using trezorctl get-features | grep passphrase_protection whether it is set to True correctly.

[grdddj]

Is the applySettings command working properly? You can find out using trezorctl get-features | grep passphrase_protection whether it is set to True correctly.

In both cases (running with 2.3.1 and 2-master) it was having passphrase_protection as False. The difference is that in newer firmwares, we are enforcing that when passphrase_always_on_device is sent into applySettings, then the passphrase_protection must be True - so the new emulators were complaining, because we were (for some reason) sending passphrase_always_on_device: False from Suite. After getting rid of this argument (which was there also by default when calling applySettings) it seems to be fine. Hopefully it will not break any other tests (let's see in CI)

Commit 18ca6ce:

• doing the changes described above
• finishing the transition to using 2-master emulators everywhere where relevant (apart from cases testing update or some explicitly old version)

[grdddj]

Observation of the CI:

• we are green right now, but only after two manual Retries of the pipeline (which was only running those stages that failed)
• therefore the flakiness is still there, cannot we somehow tell CI to retry each stage 2/3 times automatically, the same way the individual Cypress tests are mostly tried 3 times?

[mroz22]

cannot we somehow tell CI to retry each stage 2/3 times automatically

https://docs.gitlab.com/ee/ci/yaml/#retry But I don't like doing this very much

[tsusanka]

Yeah Gitlab has retry but it is a slippery slope which delays the total pipeline run significantly. On the other hand it is what I am doing manually quite often these days anyway.. Btw @grdddj could you rebase on top of current develop? It has the Cypres timeout increase so I am wondering if that would help.

[mroz22]

looks good. I think we can merge this and build upon it further on

[grdddj]

Btw @grdddj could you rebase on top of current develop?

Yeah, let me try that

looks good. I think we can merge this and build upon it further on

Should everything be squashed into one commit? Message could be something like chore(tests): replacing emulator version in e2e tests with latest master build

[grdddj]

Possible improvement:

• cannot we make the 2-master and 1-master global variables shared between all the tests, so that we have one place to change the emulator version?

Use case:

• being able to input any emulator version into the tests easily (via some script argument), and it would run all the tests using this
• connected with Use custom firmware trezor-user-env#49, where we would like to define a way of running e2e tests against a custom emulator

[grdddj]

Commit 958744f:

• implementing idea above, creating Cypress environment variables to hold used T1 and T2 emulator versions

Use-case:

• by docs these env variables can be changed by setting CYPRESS_XXX OS environmental variable or by CLI option cypress run --env XXX=YYY
• it could be nicely used by firmware CI specifying a special firmware version (even custom-built, for example sitting on some URL) it wants to run the Suite e2e tests on

[grdddj]

Fixup commit 4b69533:

• forgotten modification from debugging

[grdddj]

Bug in CI:

• the environment variables are being undefined in all tests
• it does not create error immediately, as tenv spawns the latest TT emulator when given no version
• however, it means we are currently spawning TT emulators even in T1 tests

[grdddj]

Fixup commit 4828328:

• putting the env object into the config which is (hopefully) being used by CI integration tests

[grdddj]

The only failing tests now (even after rebasing on develop) remains recovery/t2-dry-run-persistence.test.ts

It is at least consistent in its error, after reloading the page the message "Device detected in incorrect state" is there. Running this locally also experiences issues, the same as described in comment above

(In develop, the test is alright with 2.3.1 emulator)

[grdddj]

Commit 96453a2:

• trying to make the t2-dry-run-persistence test pass by changing the emulator to old 2.3.1 version, which seems to work fine in develop

Still, the test in CI is failing (as the only one). After discussion with @tsusanka I am creating an issue for the QA team to investigate if it really fails

[tsusanka]

So the 2.3.1 passes on develop but fails here? That's curious 🤔

[mroz22]

Few ideas:

• check this documentation https://github.com/trezor/trezor.js/blob/master/API.md#multitasking on how concurrency works when there are multiple applications in play. It works similary with connect too.
• unacquired device screen probably happens due to the fact that device was not released by bridge before app reloaded.
• try to put cy.wait(501) before cy.reload()

[grdddj]

Commit 9ceac66:

• skipping the unstable test as per Broken model T persistence mode e2e test #4128 (comment)

[tsusanka]

So @grdddj I think we are ready here right? Pleas squash and force-push, one commit as suggested above is fine. We'll make final review then and merge

[grdddj]

So @grdddj I think we are ready here right? Pleas squash and force-push, one commit as suggested above is fine. We'll make final review then and merge

I hope so. The squash was done and force-pushed. We can still discuss the commit message, I made it quite long

[tsusanka]

@grdddj could you please have a look why the suite/new-account-message.test.ts test is failing on AssertionError: Timed out retrying after 15000ms: Expected to find content: 'New default Accounts natively in Trezor Suite!' but never did.?

It is definitely not due to your PR, it was added in #4053. There were some troubles with CI so we could not check whether it passes :/. Sorry, I should have made sure. I know it is a bit out of scope but do you think you could have a look at it please?

[grdddj]

@grdddj could you please have a look why the suite/new-account-message.test.ts test is failing on AssertionError: Timed out retrying after 15000ms: Expected to find content: 'New default Accounts natively in Trezor Suite!' but never did.?

It is definitely not due to your PR, it was added in #4053. There were some troubles with CI so we could not check whether it passes :/. Sorry, I should have made sure. I know it is a bit out of scope but do you think you could have a look at it please?

Sure, I am looking at it. Tried to run it locally with the result below (the error message is the same as in CI). I will connect with @wendys-cats so we can troubleshoot it quicker, I am not even sure what is being tested here

[grdddj]

After changing the expected text from New default Accounts natively in Trezor Suite! to New to Trezor Suite: BTC Bech32 accounts!', it is running fine.

That text was changed in d2130d0 2 weeks ago (and that test is one month old)

There seems to be some mismatch/mystery, because the text New default Accounts natively in Trezor Suite! is present in file below:

trezor-suite/packages/suite/src/support/messages.ts

Lines 2448 to 2451 in fade0f7

	TR_BECH32_BANNER_TITLE: {
	defaultMessage: 'New default Accounts natively in Trezor Suite!',
	id: 'TR_BECH32_BANNER_TITLE',
	},

@tsusanka Should I create a PR for this fix (just changing the expected text)? We have discussed it with @wendys-cats

[tsusanka]

Ah! That is due to the fact that we modify the texts in a Crowdin. So we ideally shouldn't depend on any text at all 😞. For now could you please modify the test so it checks New to Trezor Suite: BTC Bech32 accounts!? And we need to discuss within Suite team how to approach this in general. Let's include this in this PR for simplicity! Thanks a lot!

[grdddj]

Ah! That is due to the fact that we modify the texts in a Crowdin. So we ideally shouldn't depend on any text at all disappointed. For now could you please modify the test so it checks New to Trezor Suite: BTC Bech32 accounts!? And we need to discuss within Suite team how to approach this in general. Let's include this in this PR for simplicity! Thanks a lot!

PR created. That Crowdin means we can change the text anywhere in Suite in real time even without committing the changes to the repo? When those texts were only in the repo, we could maybe import the json object into the tests and take the text dynamically from there, without hardcoding them

[tsusanka]

Not live - the texts are changed via the Crowdin interface and from time to time (usually before release) sync here, see #3937 for example. Crowdin is also used for localization into other languages, see our Notion tutorial.

[tsusanka]

Now that #4171 is merged could you rebase on top of develop and force-push? The CI should pass then so I just would like to double-check and also see if some other surprises are there :).

[tsusanka]

I went through the changes and I have only one last question :).