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

add ui tests for add / update / remove card #1843

Merged
merged 23 commits into from
Jan 19, 2022
Merged

Conversation

asnaith
Copy link
Member

@asnaith asnaith commented Jan 7, 2022

closes #1842

  • Adds new ui tests for adding, update and removing a card on the user account.

@render
Copy link

render bot commented Jan 7, 2022

@render
Copy link

render bot commented Jan 7, 2022

@render
Copy link

render bot commented Jan 7, 2022

@github-actions github-actions bot added the Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes. label Jan 7, 2022
@Tbaut
Copy link
Collaborator

Tbaut commented Jan 7, 2022

All right, after looking for things on the stripe doc, I ended up finding a pretty elegant way to actually select the input fields inside an iframe, and type in it. Thanks to this and this articles.

add-card.mp4

@asnaith
Copy link
Member Author

asnaith commented Jan 8, 2022

All right, after looking for things on the stripe doc, I ended up finding a pretty elegant way to actually select the input fields inside an iframe, and type in it. Thanks to this and this articles.

Thank you so much @Tbaut, this was a great help 🙏.

I didn't have a stripe key in my .env file either which was also causing me a problem!

@asnaith asnaith changed the title add ui tests for add card (wip) add ui tests for add / update / remove card (wip) Jan 8, 2022
@asnaith asnaith marked this pull request as ready for review January 10, 2022 22:55
@asnaith asnaith requested review from FSM1, Tbaut and tanmoyAtb January 10, 2022 22:55
@asnaith
Copy link
Member Author

asnaith commented Jan 10, 2022

I've marked this as it.skip for now until we have the ability to remove cards programmatically (#1847)

update: resolved!

@asnaith asnaith changed the title add ui tests for add / update / remove card (wip) add ui tests for add / update / remove card Jan 11, 2022
@asnaith asnaith added the Status: Review Needed 👀 Added to PRs when they need more review label Jan 11, 2022
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

I found another blog post, from cypress themselves, that goes into details, and feels a little less hacky. You can take a look, but I can help for the implementation if you want.

here: https://www.cypress.io/blog/2020/02/12/working-with-iframes-in-cypress/

packages/files-ui/cypress/support/commands.ts Outdated Show resolved Hide resolved
addOrUpdateCardModal.addCardHeader().should("be.visible")
// wait is needed to allow time for the stripe components to be editable
// eslint-disable-next-line cypress/no-unnecessary-waiting
cy.wait(1000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to be clear, I would like to get to something robust enough to get rid of those wait, or at least try hard to avoid them before letting them in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry, I noticed my comment from yesterday disappeared (it looked like it posted twice so I deleted one of them but removed both!).

I tried with the iframe timeout in place and with the waits removed and I was still having issues. I think it's because there is a momentary lag between when the elements become visible and when they become interactive (stripe elements specifically).

I'll keep experimenting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I'm not totally surprised, that's why I suggested the other blog post from cypress, where each step is explained. The issue is that our Cypress.$(getter, document) doesn't have a timeout, this is the part that will check for the intput field inside the iframe. I checked and couldn't figure a way to make this wait, so maybe we should check in details the other solution and see if it's more elegant. In the end, both end up doing the same.

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 don't think the issue was the timeout, as you'd added one and it would return a successful "be.visible" check but sometimes then fail when trying to type.

It seems to me that the stripe elements return true for visibility before it is actually possible to type in them (but I'm talking in terms of < 1 second, that's how I'd avoided it with the 1 second wait).

In the dev tools, I watched all the requests that came in after opening the add card modal and by adding a spy on all the stripe requests (the r.stripe calls which seem to be responsible for initializing/activating the elements) and waiting for them to complete I was able to get reliable input without the hardcoded wait.

That was a lot of seconds spent in dev to avoid the hardcoded 1 second wait 😅. I do think this is better though as if there's ever a failure in the future because of a failed request at least it will be more obvious to diagnose.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, @Tbaut I was talking about getting the stripe key in .env file in render before...and I just realized that doesn't really matter as we're not running these tests on render instances 🤦‍♂️.

We need to get the stripe key in the .env file in the docker container running the tests. I know that can be done via test-files.yml but I don't have repo permissions to add GitHub secrets so can't do that at the moment. Would you be able to assist or give me the appropriate access?

After that, I think we will be good to go on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

aah not sure what I was thinking of. Just added the stripe keys, although I don't think it's a secret, since it's available in the calls if you inspect them, I added it as such.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Tbaut Ah ok, thank you for doing that 🙏. Perhaps still best to keep it a little bit obscured even if the key is obtainable through looking at the calls.

@asnaith asnaith requested a review from Tbaut January 15, 2022 05:43
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

This look robust and is now passing with the stripe key! Just found a couple nits, but it's good to go.

@asnaith asnaith merged commit 7cb726c into dev Jan 19, 2022
@asnaith asnaith deleted the mnt/add-card-ui-tests-1842 branch January 19, 2022 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 👀 Added to PRs when they need more review Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add new ui tests for adding a card to profile
4 participants