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

[Files] Change language test #2192

Merged
merged 5 commits into from
Jun 16, 2022
Merged

[Files] Change language test #2192

merged 5 commits into from
Jun 16, 2022

Conversation

juans-chainsafe
Copy link
Contributor

closes #2171

@juans-chainsafe juans-chainsafe added the Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes. label Jun 15, 2022
@juans-chainsafe juans-chainsafe self-assigned this Jun 15, 2022
@render
Copy link

render bot commented Jun 15, 2022

@render
Copy link

render bot commented Jun 15, 2022

@render
Copy link

render bot commented Jun 15, 2022

@juans-chainsafe
Copy link
Contributor Author

Some subscription tests failed but not because of this PR, I already reported to API team

@asnaith
Copy link
Member

asnaith commented Jun 15, 2022

Thanks for adding this Juan 👍

I know we usually don't check element text values in tests but for a change language test I think it makes sense and what you have used seems very unlikely to change so I'm ok with it for this purpose.

I pushed a few additional commits to update identifiers, simplify assertions, and increased the test reliability by waiting on the last api response (after language select + page refresh) before the assertions. The latter helped avoid "detached from DOM" errors.

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.

nice one!

@Tbaut
Copy link
Collaborator

Tbaut commented Jun 16, 2022

Merging since this has nothing to do with the failing tests that is being taken care of

@Tbaut Tbaut merged commit 6b5287e into dev Jun 16, 2022
@Tbaut Tbaut deleted the mnt/change-language-test-2171 branch June 16, 2022 10:56
@juans-chainsafe
Copy link
Contributor Author

Thanks for adding this Juan 👍

I know we usually don't check element text values in tests but for a change language test I think it makes sense and what you have used seems very unlikely to change so I'm ok with it for this purpose.

I pushed a few additional commits to update identifiers, simplify assertions, and increased the test reliability by waiting on the last api response (after language select + page refresh) before the assertions. The latter helped avoid "detached from DOM" errors.

Thanks for the updates! now I know that I don't need the exact element to get the text, I can get it finding the parent element (I think in Selenium is not like that, because of that I didn't know that Cypress can do it).

And using the intercept for wait to finish is awesome too, Didn't think about that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

[Files] Change language test
3 participants