test: add footer and load safe #3802

francovenica · 2022-04-20T16:56:09Z

What it solves

Add e2e for cypress test to verify the footer and the load safe workflow

How to test it

Write the command yarn cypress:open dev
The cypress ui test selector opens
Select the files "load_safe" and "app_layout"

github-actions · 2022-04-20T16:56:25Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2022-04-20T17:00:02Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	2	0
Warnings	0	0
Ignored	2	N/A

Result: ❌ failure
Annotations: 2 total

[failure] @typescript-eslint/no-unused-vars

Disallow unused variables

cypress/plugins/index.js Line 19 - 'on' is defined but never used.
cypress/plugins/index.js Line 19 - 'config' is defined but never used.

^{Report generated by eslint-plus-action}

coveralls · 2022-04-20T17:06:25Z

Pull Request Test Coverage Report for Build 2197117793

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.007%) to 35.548%

Totals
Change from base Build 2195129238:	-0.007%
Covered Lines:	3554
Relevant Lines:	9044

💛 - Coveralls

github-actions · 2022-04-20T17:07:04Z

Deployment links

🟠 Rinkeby	⚪ Mainnet	🟣 Polygon	🟡 BSC	⚫ Arbitrum	🟢 Gnosis Chain

iamacook

Nice work dude! More than happy to go through my suggestions with you

iamacook · 2022-04-20T20:19:32Z

cypress/integration/app_layout.spec.js

+        cy.visit("/rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac/settings/details")
+        cy.get('footer').should('exist')
+
+        cy.visit("/rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac/transactions/history")
+        cy.get('footer').should('not.exist')
+
+        cy.visit("/rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac/balances")
+        cy.get('footer').should('not.exist')
+
+        cy.visit("/rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac/apps")


It's probably best we use the generateSafeRoute function and our pre-defined route variables here.

I disagree. IMHO the dumber the better.

iamacook · 2022-04-20T20:19:52Z

cypress/integration/app_layout.spec.js

+        cy.visit("/")
+        cy.get('footer').should('exist')
+
+        cy.visit("/rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac/settings/details")
+        cy.get('footer').should('exist')
+
+        cy.visit("/rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac/transactions/history")
+        cy.get('footer').should('not.exist')
+
+        cy.visit("/rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac/balances")
+        cy.get('footer').should('not.exist')
+
+        cy.visit("/rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac/apps")


There should all begin with the PUBLIC_URL, '/app'.

iamacook · 2022-04-20T20:21:30Z

cypress/integration/load_safe.spec.js

+        cy.wait(1000) //Have to wait because clicking the switch network fails sometimes if not
+
+        //Step 1
+        cy.get('[data-testid="select-network-step"]').find('button').click()


We should use cy.findByTestId instead of cy.get('[data-testid=....

I won't mark the other instances of this.

i would even say to find by text whenever possible

iamacook · 2022-04-20T20:22:30Z

cypress/plugins/index.js

@@ -0,0 +1,22 @@
+/// <reference types="cypress" />


Do we need this file? It doesn't seem to add anything and failing the linter.

For what I read is for the intellisense

katspaugh · 2022-04-21T06:44:06Z

cypress/plugins/index.js

+ * @type {Cypress.PluginConfig}
+ */
+// eslint-disable-next-line no-unused-vars
+module.exports = (on, config) => {


You can replace on with _, then it won't complain.

Edit: actually, just remove both on and config.

I'll remove them to see if it still works

katspaugh · 2022-04-21T06:47:06Z

cypress/integration/app_layout.spec.js

+const { createArrayTypeNode } = require("typescript")
+
+describe('App layout', () =>{
+    it('Check footer presence', () =>{


IMHO, this functionality (which pages show the footer) is too trivial to cover.
Tests run slowly, so we need to prefer more complex scenarios to fit in a reasonable time budget.
The slowest part is loading new pages, and this test does a lot of it, just to check for the footer.

katspaugh · 2022-04-21T06:48:35Z

There's a mix of single and double quotes, no new lines at the end of files but out linter didn't catch that.
I wonder if it ignores those files or we don't have prettier enabled on push?

katspaugh · 2022-04-21T06:51:19Z

cypress/integration/load_safe.spec.js

+const OWNER_ADDRESS = "0x6f965E48347AF3Df65c14CCc176A9CbeCEa0eDb5"
+
+describe("Load Safe", () => {
+    it("Load safe", () => {


it('should load an existing Safe'

katspaugh · 2022-04-21T06:53:18Z

cypress/integration/load_safe.spec.js

+    it("Load safe", () => {
+        cy.visit("/")
+        cy.findByText('Accept selection').click()
+
+        cy.get('[data-track="load-safe: Open stepper"]').click()
+        cy.findByText('Add existing Safe').should('exist')
+        cy.wait(1000) //Have to wait because clicking the switch network fails sometimes if not
+
+        //Step 1
+        cy.get('[data-testid="select-network-step"]').find('button').click()
+        cy.get('[aria-labelledby="select-network"]').should('exist')
+        cy.findByText('Ethereum').click()
+        cy.get('nav').findByText('Ethereum').should('exist')
+        cy.get('[data-testid="select-network-step"]').findByText('Ethereum').should('exist')
+        cy.get('[data-testid="select-network-step"]').find('button').click()
+        cy.findByText('Rinkeby').click()
+        cy.get('[data-track="load-safe: Continue"]').click()
+
+        //Step2
+        cy.get('[data-testid="load-safe-name-field"]').should('have.attr', 'placeholder').should('contain','rinkeby-safe')
+        cy.get('[data-testid="load-safe-name-field"]').type('Test safe name').should('have.value', 'Test safe name')
+
+        cy.get('[data-testid="load-safe-address-field"]').type('RandomText')
+        cy.findByText(INVALID_INPUT_ERROR_MSG).should('exist')
+        cy.get('[data-testid="load-safe-address-field"]').clear().type(NON_CONTRACT_ADDRESS)
+        cy.findByText(INVALID_ADDRESS_ERROR_MSG).should('exist')
+        cy.get('[data-testid="load-safe-address-field"]').clear().type(SAFE_ENS_NAME).should('have.value', SAFE_ENS_NAME_TRANSLATED)
+        cy.get('[data-testid="qr-icon"]').click()
+        cy.get('[class="paper"]').find('button').contains('Upload an image').click()
+        cy.get('[type="file"]').attachFile('../utils/files/rinkeby_safe_QR.png')
+        cy.get('[data-testid="load-safe-address-field"]').should('have.value', SAFE_QR_CODE_ADDRESS)
+        cy.get('[data-testid="safeAddress-valid-address-adornment"]').should('exist')
+        cy.wait(3000) //have to wait or after clicking next what loads is the owners of the previous valids safe, not the one from the QR code
+        cy.get('[type="submit"]').click()
+
+        //Step3
+        cy.findByPlaceholderText(OWNER_ENS_DEFAULT_NAME).parents('div[data-testid="owner-row"]').findByText(OWNER_ADDRESS)
+        cy.findByPlaceholderText(OWNER_ENS_DEFAULT_NAME).type('Test Owner Name').should('have.value', 'Test Owner Name')
+        cy.get('[data-track="load-safe: Continue"]').click()
+
+        //Review Step
+        cy.findByText('Test safe name').should('exist')
+        cy.findByText('Test Owner Name').should('exist')
+        cy.get('[data-track="load-safe: Add"]').click()
+
+        //Safe Loaded
+        cy.get('[data-testid="sidebar"]').findByText('Test safe name')
+        cy.get('[data-testid="sidebar"]').find('nav').findByText('Settings').click()
+        cy.get('[data-testid="sidebar"]').find('nav').findByText('Owners').click()
+        cy.findByText('Test Owner Name')
+    })


Looks like this scenario tests only the happy path.
It would be nice to also check possible input errors, Safes that don't exist (or on a diff network), adding and discarding owners, going back etc.
Doesn't need to be in the PR, just an idea for future.

I check for the safe address that wrong values and address that are not safes (like mm accounts).

DiogoSoaress · 2022-04-21T08:40:20Z

I wonder if it ignores those files or we don't have prettier enabled on push?

I think it was because we used the --no-verify flag. @francovenica you need to find a smoother way to work with your Windows machine and git/GitHub and add the prettier setup.

iamacook · 2022-04-21T08:55:30Z

I think it was because we used the --no-verify flag.

Prettier currently only formats /src, /public and /scripts.

DiogoSoaress · 2022-04-25T15:34:25Z

cypress/integration/load_safe.spec.js

@@ -0,0 +1,65 @@
+import 'cypress-file-upload';
+const path = require("path");


Import not used

is used to upload the QR file

DiogoSoaress · 2022-04-25T15:35:05Z

cypress/integration/load_safe.spec.js

+const SAFE_ENS_NAME = "safe.test"
+const SAFE_ENS_NAME_TRANSLATED = "0x83eC7B0506556a7749306D69681aDbDbd08f0769"
+const SAFE_QR_CODE_ADDRESS = "0x9913B9180C20C6b0F21B6480c84422F6ebc4B808"
+const NON_CONTRACT_ADDRESS = "0x61a0c717d18232711bC788F19C9Cd56a43cc8872"
+const INVALID_INPUT_ERROR_MSG = "Must be a valid address, ENS or Unstoppable domain"
+const INVALID_ADDRESS_ERROR_MSG = "Address given is not a valid Safe address"
+const OWNER_ENS_DEFAULT_NAME = "francoledger.eth"
+const OWNER_ADDRESS = "0x6f965E48347AF3Df65c14CCc176A9CbeCEa0eDb5"


Suggest moving to a constants.ts file

I prefer to create a bunch of test that work individually first. At some point as the test grow I'll create files to share constant, selectors and such.

DiogoSoaress · 2022-04-25T15:38:40Z

cypress/integration/load_safe.spec.js

+
+        cy.get('[data-track="load-safe: Open stepper"]').click()
+        cy.findByText('Add existing Safe').should('exist')
+        cy.wait(1000) //Have to wait because clicking the switch network fails sometimes if not


can we wait for data-testid="select-network-step" to exist? as opposite to timeouts

The problem is the application, if you click too fast it fails to load. Is not that the button is missing or anything, is that you can click it before it loads properly

DiogoSoaress · 2022-04-25T15:44:52Z

cypress/integration/load_safe.spec.js

+        cy.wait(1000) //Have to wait because clicking the switch network fails sometimes if not
+
+        //Step 1
+        cy.get('[data-testid="select-network-step"]').find('button').click()


i would even say to find by text whenever possible

DiogoSoaress · 2022-04-25T15:46:00Z

cypress/integration/load_safe.spec.js

+        cy.findByText('Ethereum').click()
+        cy.get('nav').findByText('Ethereum').should('exist')
+        cy.get('[data-testid="select-network-step"]').findByText('Ethereum').should('exist')
+        cy.get('[data-testid="select-network-step"]').find('button').click()


Are we changing network unnecessarily? I would keep the test as straightforward as possible and do not bloat to much the tested scenario

Just making sure the network works back and forth. I'll do the same whenever we can make a safe creation test

DiogoSoaress · 2022-04-25T15:48:24Z

cypress/integration/load_safe.spec.js

+        cy.findByText(INVALID_ADDRESS_ERROR_MSG).should('exist')
+        cy.get('[data-testid="load-safe-address-field"]').clear().type(SAFE_ENS_NAME).should('have.value', SAFE_ENS_NAME_TRANSLATED)
+        cy.get('[data-testid="qr-icon"]').click()
+        cy.get('[class="paper"]').find('button').contains('Upload an image').click()


Does findByText work here?

DiogoSoaress · 2022-04-25T15:50:37Z

cypress/integration/load_safe.spec.js

+        cy.get('[type="file"]').attachFile('../utils/files/rinkeby_safe_QR.png')
+        cy.get('[data-testid="load-safe-address-field"]').should('have.value', SAFE_QR_CODE_ADDRESS)
+        cy.get('[data-testid="safeAddress-valid-address-adornment"]').should('exist')
+        cy.wait(3000) //have to wait or after clicking next what loads is the owners of the previous valids safe, not the one from the QR code


Can we wait for the submit button not to be disabled instead of a timeout?

The problem here is that if you clcik the button too fast when it reaches the owner steps it shows the owners for the safe that was loaded previously and not the safe loaded by the QR code. Is a case of the app not ready to have a user clicking the "next" button so fast

DiogoSoaress · 2022-04-25T15:51:18Z

cypress/integration/load_safe.spec.js

+        //Review Step
+        cy.findByText('Test safe name').should('exist')
+        cy.findByText('Test Owner Name').should('exist')
+        cy.get('[data-track="load-safe: Add"]').click()


findByText

DiogoSoaress · 2022-04-25T15:53:36Z

cypress/integration/load_safe.spec.js

+        //Step3
+        cy.findByPlaceholderText(OWNER_ENS_DEFAULT_NAME).parents('div[data-testid="owner-row"]').findByText(OWNER_ADDRESS)
+        cy.findByPlaceholderText(OWNER_ENS_DEFAULT_NAME).type('Test Owner Name').should('have.value', 'Test Owner Name')
+        cy.get('[data-track="load-safe: Continue"]').click()


findByText

francovenica · 2022-05-02T03:11:13Z

On a personal note, I'd not like to use the "findByText" as the first option, since that searches for the first occurrence of a text, so if it is not unique you are in trouble. I'd prefer to use any other selector as first option (a test-id, a class, an alt or title...)
A better way to use it is if the scope is reduced, like searching for a specific text on the sidebar only:
cy.FindByTestId('sidebar').findByText('Some text here')

github-actions · 2022-05-02T12:53:49Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	2	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

Fixed

test: add footer and load safe

4c0cb4e

francovenica requested review from DiogoSoaress and katspaugh April 20, 2022 16:57

iamacook reviewed Apr 20, 2022

View reviewed changes

katspaugh reviewed Apr 21, 2022

View reviewed changes

DiogoSoaress previously requested changes Apr 25, 2022

View reviewed changes

Replace get for findByTextId and index file

cf6237b

replace single quotes

ce0a95c

katspaugh approved these changes May 2, 2022

View reviewed changes

francovenica merged commit ca53029 into dev May 3, 2022

francovenica deleted the test-footer-and-load-safe branch May 3, 2022 13:47

github-actions bot locked and limited conversation to collaborators May 3, 2022

		@@ -0,0 +1,65 @@
		import 'cypress-file-upload';
		const path = require("path");

test: add footer and load safe #3802

test: add footer and load safe #3802

Conversation

francovenica commented Apr 20, 2022

What it solves

How to test it

github-actions bot commented Apr 20, 2022

github-actions bot commented Apr 20, 2022

ESLint Summary View Full Report

[failure] @typescript-eslint/no-unused-vars

coveralls commented Apr 20, 2022

Pull Request Test Coverage Report for Build 2197117793

💛 - Coveralls

github-actions bot commented Apr 20, 2022

Deployment links

iamacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katspaugh Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katspaugh Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

katspaugh commented Apr 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiogoSoaress commented Apr 21, 2022 • edited Loading

iamacook commented Apr 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francovenica commented May 2, 2022

github-actions bot commented May 2, 2022 • edited Loading

ESLint Summary View Full Report

katspaugh Apr 21, 2022 •

edited

Loading

katspaugh Apr 21, 2022 •

edited

Loading

DiogoSoaress commented Apr 21, 2022 •

edited

Loading

github-actions bot commented May 2, 2022 •

edited

Loading