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

test: Complete missing step for add a contact to the address book in existing E2E test #27959

Merged

Conversation

benjisclowder
Copy link
Contributor

@benjisclowder benjisclowder commented Oct 18, 2024

Description

Adding missing "add contact" step to this E2E test test/e2e/tests/settings/address-book.spec.js to fully cover the manual scenario here.

Open in GitHub Codespaces

Related issues

Fixes: #27369

Manual testing steps

  1. Run yarn test:e2e:single test/e2e/tests/settings/address-book.spec.js --browser=chrome --leave-running
  2. Test should pass with no failure
  3. The second test run in the test suite is the one added in this PR

Note: I have used the class instead of specific selectors (text+tag) for the element as the latter (Add contact/button) would not be located when running the test and the test would consistently fail.

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@benjisclowder benjisclowder added team-extension-platform area-qa Relating to QA work (Quality Assurance) labels Oct 18, 2024
@benjisclowder benjisclowder self-assigned this Oct 18, 2024
Copy link
Contributor

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.

@benjisclowder benjisclowder marked this pull request as ready for review October 18, 2024 13:57
@benjisclowder benjisclowder requested a review from a team as a code owner October 18, 2024 13:57
@metamaskbot
Copy link
Collaborator

Builds ready [dfc8f17]
Page Load Metrics (1795 ± 111 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint157025981800241115
domContentLoaded152525301759232111
load156925411795231111
domInteractive16199544522
backgroundConnect8163403818
firstReactRender452851006029
getState584202110
initialActions00000
loadScripts107021211308238114
setupStore1093392813
uiStartup170927522029265127
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@hjetpoluru hjetpoluru left a comment

Choose a reason for hiding this comment

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

Nice effort, @benjisclowder! I have added some feedback and potential replacements.

I notice that the file itself has a lot of anti-patterns. Once these changes are made, if you feel comfortable, update this spec file and try to adapt the POM.

test/e2e/tests/settings/address-book.spec.js Outdated Show resolved Hide resolved
benjisclowder and others added 4 commits October 23, 2024 17:48
Co-authored-by: Harika <153644847+hjetpoluru@users.noreply.github.com>
Co-authored-by: Harika <153644847+hjetpoluru@users.noreply.github.com>
…thub.com:MetaMask/metamask-extension into test--adding-additional-step-to-address-book-E2E
@hjetpoluru
Copy link
Contributor

@benjisclowder, code looks good now. Just fix the lint errors and get the latest develop and will approve the PR.

@chloeYue
Copy link
Contributor

Hi @benjisclowder , could you remove the added submodule as it's not necessary? thanks !
Screenshot 2024-10-24 at 10 28 31

@metamaskbot
Copy link
Collaborator

Builds ready [81a2c2c]
Page Load Metrics (2123 ± 86 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32026082022425204
domContentLoaded17912409207816680
load18642588212317986
domInteractive308751178
backgroundConnect8196504220
firstReactRender642221143517
getState585252512
initialActions01000
loadScripts13081814153513967
setupStore12103332612
uiStartup204028652392214103
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@benjisclowder
Copy link
Contributor Author

Hi, @hjetpoluru and @chloeYue! Thanks a lot for the feedback and suggestions. All changes have been made and PR is ready for approval.

@hjetpoluru hjetpoluru self-requested a review October 24, 2024 20:12
Copy link
Contributor

@hjetpoluru hjetpoluru left a comment

Choose a reason for hiding this comment

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

LGTM

@benjisclowder benjisclowder added this pull request to the merge queue Oct 25, 2024
Merged via the queue into develop with commit a6734d6 Oct 25, 2024
76 checks passed
@benjisclowder benjisclowder deleted the test--adding-additional-step-to-address-book-E2E branch October 25, 2024 10:41
@github-actions github-actions bot locked and limited conversation to collaborators Oct 25, 2024
@metamaskbot metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-qa Relating to QA work (Quality Assurance) release-12.7.0 Issue or pull request that will be included in release 12.7.0 team-extension-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test: Complete missing step for add a contact to the address book in existing E2E test
4 participants