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

[FIX] Appium: Notify appium server test is done #6349

Merged
merged 1 commit into from
May 9, 2023

Conversation

chrisleewilcox
Copy link
Contributor

@chrisleewilcox chrisleewilcox commented May 7, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description
Some tests will fail after test run even if all steps pass which causes issues with results and reports. This was due to our wdio automation not notifying appium that the test has completed, which would cause some callbacks to fail after the test because the callbacks refer to a test sessionID that no longer exist because the sessionID was terminated after the test. You can see the bug by looking at the bitrise logs that is linked to the issue reported.

This change will notify appium that a sessionID has completed and any outstanding callbacks that reference sessionID's that no longer exist will be handled by appium and no failure/error is generated after the test.

Fixes: reporting a failed test after test run even though all test steps passed, reporting the test correctly.
Fixes: callbacks that refer to a terminated/expired test sessionID to be handle without reporting a failure.

Screenshots/Recordings
Bitrise report for all test.
Smoke test on browserstack with CreateNewWallet.feature that was consistently getting the error. All smoke tests pass image.

Issue

Progresses #949
Progresses #5845

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@chrisleewilcox chrisleewilcox requested a review from a team as a code owner May 7, 2023 21:56
@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2023

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@gauthierpetetin gauthierpetetin added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 8, 2023
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@sethkfman sethkfman removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 8, 2023
@chrisleewilcox chrisleewilcox merged commit 64a9d5c into release/6.6.0 May 9, 2023
19 checks passed
@chrisleewilcox chrisleewilcox deleted the appium/notify-appium-test-done branch May 9, 2023 02:51
@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants