Skip to content

renamed clickSentButton to clickSendButton, updated test after change… - #1060

Merged
tomholub merged 8 commits into
masterfrom
tests/issue-1049
Nov 24, 2021
Merged

tomholub merged 8 commits into
masterfrom
tests/issue-1049

Conversation

@fcvakintos

@fcvakintos fcvakintos commented Nov 22, 2021 •

Copy link
Copy Markdown
Contributor

…s, added fixes for checkEmail in message

This PR contains changes for checking sender email, renamed clickSentButton to clickSendButton

close #1049


Tests

  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@tomholub tomholub left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you - see comment.


checkEmailAddress = async (email: string) => {
const selector = `~${email}`;
const selector = `-ios class chain:**/XCUIElementTypeStaticText[\`label == "${email}"\`]`;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Whenever you see yourself using this kind of complicated selector, please add accessibility identifier instead, and query by that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will try

Comment on lines 55 to 56
checkEmailAddress = async (email: string) => {
const selector = `-ios class chain:**/XCUIElementTypeStaticText[\`label == "${email}"\`]`;
const selector = `~${email}`;
await (await $(selector)).waitForDisplayed();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What I meant is, an accessibility identifier should be added to the sender element on message screen in Swift. Then query here by that identifier.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

eg

Error: element ("~e2e.enterprise.test@flowcrypt.com") still not displayed after 15000ms
    at async EmailScreen.checkEmailAddress (/Users/tom/git/flowcrypt-ios/appium/tests/screenobjects/email.screen.ts:56:5)
    at async EmailScreen.checkOpenedEmail (/Users/tom/git/flowcrypt-ios/appium/tests/screenobjects/email.screen.ts:71:5)
    at async UserContext.<anonymous> (/Users/tom/git/flowcrypt-ios/appium/tests/specs/composeEmail/SendEncryptedEmailAfterPassPhraseSessionEndedAndTrashIt.spec.ts:57:5)

When in fact I see it on screen. By using accessibility identifiers instead, we remove uncertainty.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added senderEmail accessibilityId

@fcvakintos
fcvakintos requested a review from tomholub November 24, 2021 08:42
@fcvakintos

Copy link
Copy Markdown
Contributor Author

@tomholub it is failed because we didn't push new accessibilityId in master
https://flowcrypt.semaphoreci.com/jobs/fd396163-6d60-47d2-9b8f-c4b482b9b6a1

Comment on lines +79 to +80
senderNode.accessibilityIdentifier = "date"

@tomholub tomholub Nov 24, 2021 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

here you meant dateNode.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no,
senderNode.accessibilityIdentifier = "senderEmail"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please carefully re-read the code I quoted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, sorry, fixed already,
will push

@tomholub

Copy link
Copy Markdown
Collaborator

@tomholub it is failed because we didn't push new accessibilityId in master https://flowcrypt.semaphoreci.com/jobs/fd396163-6d60-47d2-9b8f-c4b482b9b6a1

Can you be more specific - which PR needs to be merged for this to pass?

@fcvakintos

Copy link
Copy Markdown
Contributor Author

@tomholub we need to merge this selector
senderNode.accessibilityIdentifier = "senderEmail"

@tomholub

Copy link
Copy Markdown
Collaborator

@tomholub we need to merge this selector senderNode.accessibilityIdentifier = "senderEmail"

We don't. Appium tests are not testing master (hopefully!!), they are testing the code in this PR. If you just changed an accessibility identifier, it must be directly reflected in tests that run in this PR.

@tomholub tomholub closed this Nov 24, 2021
@tomholub tomholub reopened this Nov 24, 2021
@fcvakintos
fcvakintos requested a review from tomholub November 24, 2021 12:53
@tomholub
tomholub enabled auto-merge (squash) November 24, 2021 13:00
@tomholub
tomholub merged commit 7d0518d into master Nov 24, 2021
@tomholub
tomholub deleted the tests/issue-1049 branch November 24, 2021 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

clickSentButton should be called clickSendButton

3 participants