feat: Add connect command

[Lykhoyda]

closes #371

Compatibility

• Functionality of change validated with a connected account with multisig
• Applicable elements hidden/disabled for watched multisigs / pure
• Looks good for solo multisig
• Looks good for multisig with proxy

[asnaith]

Hey @Lykhoyda, I noticed some of the tests that I recently merged failed in this branch due to slower loading when executing in CI:

I pushed a tiny commit to this branch to increase the timeout for the account header element. This is the element the test is looking for after navigating to the home page but we can see from the screenshot it's still loading so couldn't find it, and exceeded the default timeout we were relying on (4 seconds).

[asnaith]

This could just be topMenuItems.newMultisigButton().click()

[asnaith]

Personally, I do like the thoroughness of the check of the string but in the past (before your time, maybe even on another FE project!) I recall there was a group consensus that we should just check for the presence of the error element itself but not the text so this is what I've been doing since.

[Tbaut]

Agreed as long as this container is unique, and only this error would be displayed, in it. In this case, the container is called noAccount.. So we should be fine. Another idea could be that we keep the text check, but only check that it contains "no account found"? So that if we even change the rest, we won't have to change this test?

[Lykhoyda]

@asnaith @Tbaut I have mixed feelings about the way of writing e2e tests. If we write the test from the expectation of the user what should happen on the screen, then we indeed carry about the text that will be displayed on the screen. And if we change the text on the UI side the tests would fail which is a valid scenario for me. The way we wrote the current test is we found the element by class and checked for visibility, it's an implementation details test as a user will not deal with the classes or any code at all, the user will see only text. BTW, it's also why I like testing library(https://testing-library.com/docs/) because it interacts only with displayed content by the core principles, and using the main API will not allow you to write such a test.

[Tbaut]

It's just a matter of how safe we are with what's on the screen, and what maintenance burden this adds. At the end of the day, you and me are the one who will break the tests if we change the text, so we're the one who may be annoyed. The actual best way is to export those strings to shared files, but this is overkill IMHO. Let's keep this string check if you feel strongly about it, I doubt it will be such a maintenance annoyance. And if it it, we can reconsider this practice in the future.

[Lykhoyda]

It's not only about checking the strings but keeping in mind the way to write the test, that it should be user-centric. The Cypress default library is super flexible, but from the beginning, it's better to have some guidance and principles for writing the test, and IMO the testing library forces good practices to avoid testing the implementation details. I want that we will all be on the same page. Let me know if you are against testing according to the following principles https://testing-library.com/docs/guiding-principles

[Tbaut]

I don't have strong opinions. Also tbh I'm not sure to fully understand the content of the link you posted as it's written in a very abstract way 😄 . I just want our tests to be easy to read/understand, easy to maintain, while obviously being meaningful. I do think it's meaningful whether we check the string or check that this unique component with the error is visible (Cypress is clever enough that if it's present, but not visible, the test will fail). Both have pros/cons. So far in other projects, we didn't check the string because it was easier to maintain and we were 100% sure that if this component is visible, then this message is visible. Now I understand your PoV too, it's safer still to check the string. To go half way, that's why I proposed instead of checking the whole string, to just check the most important part, and the part that will likely not change "No account found". Because if e.g we change the link in the future, I feel it'd be better to not break the test for this.

[Lykhoyda]

At least to use "contain.text" is definitely better. The code is updated

[asnaith]

@Lykhoyda I'm 100% aligned with you that the cypress tests should be user-centric. Meeting halfway with the "contains" and not an exact match is for sure better than not checking the text at all 👍

[asnaith]

import { accountDisplay } from '../support/page-objects/components/accountDisplay'

and

Suggested change

	const accountLabels = cy.get('[data-cy="label-account-name"]')
	accountLabels.each((el, index) => {
	const expectedName = Object.values(injectedAccounts)[index].name
	cy.wrap(el).should('have.text', expectedName)
	})
	accountDisplay.nameLabel().each((el, index) => {
	const expectedName = Object.values(injectedAccounts)[index].name
	cy.wrap(el).should('have.text', expectedName)
	})

[asnaith]

Suggested change

	/**
	* Connect an accounts to the extension
	* @param accountAddress
	*/
	/**
	* Connect accounts to the dApp
	* @param accountAddresses
	*/

[Tbaut]

Looks good to me.

Just a nit for this PR, please either remove the irrelevant checkboxs in the first post, or check them. Leaving them unchecked makes things unclear for anyone who's unaware.