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.

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

test: E2E create queued transactions #948

Merged
merged 10 commits into from
Oct 25, 2022
Merged

test: E2E create queued transactions #948

merged 10 commits into from
Oct 25, 2022

Conversation

DiogoSoaress
Copy link
Member

@DiogoSoaress DiogoSoaress commented Oct 20, 2022

Creates a transaction and verifies it is queued

@github-actions
Copy link

github-actions bot commented Oct 20, 2022

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 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@cloudflare-pages
Copy link

cloudflare-pages bot commented Oct 20, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 154f216
Status: ✅  Deploy successful!
Preview URL: https://2a2a7903.web-core.pages.dev
Branch Preview URL: https://e2e-create-queue-tx.web-core.pages.dev

View logs

@DiogoSoaress DiogoSoaress marked this pull request as ready for review October 24, 2022 09:30
@DiogoSoaress DiogoSoaress changed the title [WIP] test: E2E create queued transactions test: E2E create queued transactions Oct 24, 2022
Copy link
Member

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

Is it not possible to target these elements via the UI, e.g. find text and traverse? Targetting via classes is on par with test IDs. I would suggest we only do so as a final option.

@DiogoSoaress
Copy link
Member Author

Is it not possible to target these elements via the UI, e.g. find text and traverse? Targetting via classes is on par with test IDs. I would suggest we only do so as a final option.

I didn't find an easy way for the check box for instance.
I rather target the class than doing something based on the DOM position. Were you commenting more in general or any selector in specific?

@DiogoSoaress
Copy link
Member Author

DiogoSoaress commented Oct 24, 2022

e.g. find text and traverse

This approach is more prone to break IMO. I tried to minimize relying on class selectors or test ids but I also tried to be pragmatic on evaluating the specificity and the code added to select a certain DOM element.

Ok, I think I've figured it out. Thanks for the feedback @iamacook

@DiogoSoaress DiogoSoaress self-assigned this Oct 24, 2022
const SAFE = 'gor:0x04f8b1EA3cBB315b87ced0E32deb5a43cC151a91'
const EOA = '0xE297437d6b53890cbf004e401F3acc67c8b39665'

describe('Queue a transaction on N/1', () => {
Copy link
Member

Choose a reason for hiding this comment

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

N/1 means N owners out of 1? Why not just 1/1 then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant 1/N

Comment on lines +62 to +68
.parent('span')
.should(($div) => {
// Turn the classList into a string
const classListString = Array.from($div[0].classList).join()
Copy link
Member

Choose a reason for hiding this comment

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

Why this instead of just checking that the input itself is checked=true?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to rely on input's checked attribute first but the HTML input is always checked. The only thing that changes on toggle is the MUICheckbox class list.

cy.contains('This Safe has no queued transactions').should('not.exist')

// Created transaction should be queued
cy.contains(`a[href="/transactions/queue?safe=${SAFE}"]`, '3' + 'Send' + '-' + '0.00003 GOR' + '1/1').should(
Copy link
Member

Choose a reason for hiding this comment

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

This check doesn't make sense if you constantly create the same transaction. 0.00003 GOR will always match a tx from previous test runs. You need to check something unique, like the nonce, or send a unique amount each time.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, then I will change the sent amount. Reason being we show the first 3 queued transactions so anything queued past 3 transactions won't be displayed in the Dashboard and if we would to verify in the Transaction List, we would also have to deal with the pagination limit.

// Alias for New transaction modal
cy.contains('h2', 'Review transaction').parents('div').as('modal')

cy.contains('Signing the transaction with nonce 4').click()
Copy link
Member

Choose a reason for hiding this comment

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

4 will be 5, 6, 7, etc in the next test runs.

Copy link
Member Author

Choose a reason for hiding this comment

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

The test keeps queuing with nonce 3 for the reason I explained in the previous comment.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how your other comment is related. The dashboard displays the oldest txns, I get that. How does the nonce of a new tx depend on that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because all the test is doing is rewriting the tx with the nonce 3.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, I've taken this approach of not increasing the queued nonce so the test could keep veryfying the Pending queued on the Dashboard

// Alias for New transaction modal
cy.contains('h2', 'Review transaction').parents('div').as('modal')

cy.contains('Signing the transaction with nonce 4').click()
Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks.

@DiogoSoaress DiogoSoaress merged commit 905fe86 into dev Oct 25, 2022
@DiogoSoaress DiogoSoaress deleted the e2e-create-queue-tx branch October 25, 2022 13:14
@gitpoap-bot
Copy link

gitpoap-bot bot commented Oct 25, 2022

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 Safe Web Core Contributor:

GitPOAP: 2022 Safe Web Core Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

schmanu pushed a commit that referenced this pull request Oct 26, 2022
* create queued tx 1/n threshold

* verify the created transaction is queued

* move create tx test to the ci folder

* add more resilient navigation

* wait for wallet loaded on CI

* queue a new transaction

* parse bette the string

* fix comments

* do not target elements by class

* randomize queued transaction value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants