Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: [POM] Migrate token tests #29375
base: main
Are you sure you want to change the base?
test: [POM] Migrate token tests #29375
Changes from 24 commits
c5367d0
6e9112d
20ba9c4
352dd40
886cf1a
874c128
d00f8e3
5aa73cf
bab4345
073579f
b4d3fec
e8fbb7a
f11c25d
be104ee
faffc73
f8a0e0e
10f2f7a
37cbee3
7f54477
3c2f970
b0febde
86ae434
8ea7007
6be682d
a8ab1ad
c166dff
c09fd7d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm this is an anti-pattern that opens the door to race conditions, where the element is rendered but it does not have yet the value we expect.
We should try to avoid finding an element and then getting its text where possible. I think in this case it's possible to avoid this. In the specs below, we should directly try to find the element by its expected inner value instead of this assert
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I agree with you here. I have used
isElementPresent
to check for the existence of the element for a given token addressThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of small things:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also had the same thought. It could make sense (if we keep it text based as I have done here)
the only doubt that I have is the depending on the token type different buttons are displayed so that is still something to think about. I do agree though there is a way we could make this easier to deal with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using text based selectors here, it's more accessible and means we don't have to create multiple selectors depending on the type of token screen we are looking at. i.e Is the token enabled for swap, bridge, send etc.
The other selector would be either
coin-overview-*
ortoken-overview-*
oreth-overview-*
This would cause flakiness in the e2e test. I do think there is a bug on the correct selector/element being rendered. It is not affecting the user but they might need to cover it with unit tests so something else does not break