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

Jump to bottom

feat: Add connect command #402

Merged
merged 8 commits into from
Oct 17, 2023
Merged

feat: Add connect command #402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7df4b85
Add connect command
Lykhoyda Oct 13, 2023
4940bd1
updates
Lykhoyda Oct 16, 2023
50f4f79
update
Lykhoyda Oct 16, 2023
969b0da
rename
Lykhoyda Oct 16, 2023
df05ae6
remove download
Lykhoyda Oct 16, 2023
183732e
remove id
Lykhoyda Oct 16, 2023
7dbfdc2
adjust comment
Lykhoyda Oct 16, 2023
5866028
contain text update
Lykhoyda Oct 16, 2023
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
24 changes: 23 additions & 1 deletion packages/ui/cypress/support/commands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

import { AuthRequests, Extension, TxRequests } from './Extension'
import { MultisigInfo, rejectCurrentMultisigTxs } from '../utils/rejectCurrentMultisigTxs'
import { InjectedAccountWitMnemonic } from '../fixtures/injectedAccounts'
import { injectedAccounts, InjectedAccountWitMnemonic } from '../fixtures/injectedAccounts'
import { topMenuItems } from './page-objects/topMenuItems'
import 'cypress-wait-until'

// ***********************************************
Expand Down Expand Up @@ -43,6 +44,7 @@ import 'cypress-wait-until'
// }

const extension = new Extension()
const Account1 = Object.values(injectedAccounts)[0].address

Cypress.Commands.add('initExtension', (accounts: InjectedAccountWitMnemonic[]) => {
cy.log('Initializing extension')
Expand All @@ -59,6 +61,20 @@ Cypress.Commands.add('getAuthRequests', () => {
return cy.wrap(extension.getAuthRequests())
})

Cypress.Commands.add('connectAccounts', (accountAddresses = [Account1] as string[]) => {
cy.getAuthRequests().then((authRequests) => {
const requests = Object.values(authRequests)
// we should have 1 connection request to the extension
cy.wrap(requests.length).should('eq', 1)
// this request should be from the application Multix
cy.wrap(requests[0].origin).should('eq', 'Multix')
// let's allow Accounts to connect
cy.enableAuth(requests[0].id, accountAddresses)
// the ui should then move on to connecting to the rpcs
topMenuItems.multiproxySelector().should('be.visible')
})
})

Cypress.Commands.add('enableAuth', (id: number, accountAddresses: string[]) => {
return extension.enableAuth(id, accountAddresses)
})
Expand Down Expand Up @@ -97,6 +113,12 @@ declare global {
*/
getAuthRequests: () => Chainable<AuthRequests>

/**
* Connect an accounts to the dApp
* @param accountAddresses
*/
connectAccounts: (accountAddresses?: string[]) => void

/**
* Authorize a specific request
* @param {number} id - the id of the request to authorize. This id is part of the getAuthRequests object response.
Expand Down
3 changes: 3 additions & 0 deletions packages/ui/cypress/support/page-objects/newMultisigPage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const newMultisigPage = {
addressSelector: () => cy.get('[data-cy="input-account-address"]')
}
2 changes: 1 addition & 1 deletion packages/ui/cypress/support/page-objects/topMenuItems.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export const topMenuItems = {
homeButton: () => cy.get('[data-cy=button-navigate-home]'),
newMultisigButton: () => cy.get('[data-cy=button-new-multisig]'),
newMultisigButton: () => cy.get('[data-cy=button-navigate-new-multisig]'),
settingsButton: () => cy.get('[data-cy=button-navigate-settings]'),
overviewButton: () => cy.get('[data-cy=button-navigate-overview]'),
aboutButton: () => cy.get('[data-cy=button-navigate-about]'),
Expand Down
34 changes: 21 additions & 13 deletions packages/ui/cypress/tests/login.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { landingPageUrl } from '../fixtures/landingData'
import { landingPage } from '../support/page-objects/landingPage'
import { topMenuItems } from '../support/page-objects/topMenuItems'
import { clickOnConnect } from '../utils/clickOnConnect'
import { newMultisigPage } from '../support/page-objects/newMultisigPage'
import { accountDisplay } from '../support/page-objects/components/accountDisplay'

describe('Connect Account', () => {
beforeEach(() => {
Expand All @@ -21,22 +23,28 @@ describe('Connect Account', () => {
// let's allow it for Alice
cy.rejectAuth(requests[0].id, 'Cancelled')
// the ui should then move on to connecting to the rpcs
landingPage.noAccountFoundError().should('be.visible')
landingPage
.noAccountFoundError()
.should(
'contain.text',
'No account found. Please connect at least one in a wallet extension.'
)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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 👍

})
})

it('Connects with Alice', () => {
const AliceAddress = Object.values(injectedAccounts)[0].address
cy.getAuthRequests().then((authRequests) => {
const requests = Object.values(authRequests)
// we should have 1 connection request to the extension
cy.wrap(requests.length).should('eq', 1)
// this request should be from the application Multix
cy.wrap(requests[0].origin).should('eq', 'Multix')
// let's allow it for Alice
cy.enableAuth(requests[0].id, [AliceAddress])
// the ui should then move on to connecting to the rpcs
topMenuItems.multiproxySelector().should('be.visible')
it('Connect Accounts', () => {
const { address: address1 } = Object.values(injectedAccounts)[0]
const { address: address2 } = Object.values(injectedAccounts)[1]

cy.connectAccounts([address1, address2])

topMenuItems.newMultisigButton().click()
// Click on the account address selector to show the list of accounts
newMultisigPage.addressSelector().click()

accountDisplay.nameLabel().each((el, index) => {
const expectedName = Object.values(injectedAccounts)[index].name
cy.wrap(el).should('have.text', expectedName)
})
})
})
12 changes: 1 addition & 11 deletions packages/ui/cypress/tests/transactions.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,7 @@ describe('Perform transactions', () => {
cy.visit(landingPageUrl)
cy.initExtension(injectedAccounts)
clickOnConnect()
cy.getAuthRequests().then((authRequests) => {
const requests = Object.values(authRequests)
// we should have 1 connection request to the extension
cy.wrap(requests.length).should('eq', 1)
// this request should be from the application Multix
cy.wrap(requests[0].origin).should('eq', 'Multix')
// let's allow it for Alice
cy.enableAuth(requests[0].id, [AliceAddress])
// the ui should then move on to connecting to the rpcs
topMenuItems.multiproxySelector().should('be.visible')
})
cy.connectAccounts([AliceAddress])
})

it('Abort a tx with Alice', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/ui/cypress/tests/watched-accounts.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe('Watched Accounts', () => {
settingsPage.accountContainer().should('have.length', 0)
})

it('can see error when attemping to add same address more than once', () => {
it('can see error when attempting to add same address more than once', () => {
// add an account first
cy.visit(settingsPageWatchAccountUrl)
addWatchAccount(addresses.Alice, 'Alice')
Expand Down
Loading