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: Extract assertion logic from the helpers.js file and add it to an Assertions class. #7512

Merged
merged 7 commits into from
Oct 18, 2023

Conversation

cortisiko
Copy link
Member

Description

Problem

To address the issue of the helpers.js file containing numerous test actions, and assertions, which can be confusing for newcomers to the framework, one possible solution is to refactor the code by extracting the assertions into a separate assertion class. This will make it easier to identify and use the assertions within e2e the tests.

Solution

Create a new file called assertions.js or any appropriate name for the assertion class. The goal here is to remove the assertions in these lines of code: https://github.com/MetaMask/metamask-mobile/blob/main/e2e/helpers.js#L121#L164 to its own assertion class.

In the assertions.js file, define a class that contains reusable assertion methods.

Important: The introduced code will remain unused. This pull request is 3 of 3 PRs, which involves cleaning up the helpers.js file.

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change.

Before

[screenshot]

After

[screenshot]

Related issues

Fixes #???

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained:
    • What problem this PR is solving.
    • How this problem was solved.
    • How reviewers can test my changes.
  • I’ve indicated what issue this PR is linked to: Fixes #???
  • I’ve included tests if applicable.
  • I’ve documented any added code.
  • I’ve applied the right labels on the PR (see labeling guidelines).
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

@cortisiko cortisiko added No QA Needed/E2E Only Apply this label when your PR does not need any QA effort. team-mobile-client E2E labels Oct 16, 2023
@cortisiko cortisiko requested a review from a team as a code owner October 16, 2023 23:48
SamuelSalas
SamuelSalas previously approved these changes Oct 17, 2023
@SamuelSalas
Copy link
Member

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (b3f0e31) 34.62% compared to head (120dc41) 34.61%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7512      +/-   ##
==========================================
- Coverage   34.62%   34.61%   -0.01%     
==========================================
  Files        1019     1019              
  Lines       27192    27193       +1     
  Branches     2217     2218       +1     
==========================================
- Hits         9414     9413       -1     
- Misses      17287    17289       +2     
  Partials      491      491              

see 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

leotm
leotm previously approved these changes Oct 17, 2023
Copy link
Contributor

@leotm leotm left a comment

Choose a reason for hiding this comment

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

lgtm

e2e/utils/Assertions.js Outdated Show resolved Hide resolved
e2e/utils/Assertions.js Outdated Show resolved Hide resolved
e2e/utils/Assertions.js Outdated Show resolved Hide resolved
Co-authored-by: LeoTM <1881059+leotm@users.noreply.github.com>
cortisiko and others added 2 commits October 17, 2023 11:37
Co-authored-by: LeoTM <1881059+leotm@users.noreply.github.com>
Co-authored-by: LeoTM <1881059+leotm@users.noreply.github.com>
@cortisiko
Copy link
Member Author

added your suggestions @leotm ! please review it one more time.

leotm
leotm previously approved these changes Oct 17, 2023
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

Can we update one test file that uses one of the assertions in the new file to show how/it works?

@sonarcloud
Copy link

sonarcloud bot commented Oct 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@cortisiko
Copy link
Member Author

@chrisleewilcox I didn't want to merge the example into this PR because it would have required time to restructure a few pages. I branched off this branch and created an example. See example here: main...detox/example-pom#diff-1eb196708d872982bf1ba97d28e5c5a0fa11db2535e052c32e60139bf422044dR30

If it makes sense to you, can you review and approve this PR

@cortisiko cortisiko merged commit 298622f into main Oct 18, 2023
25 checks passed
@cortisiko cortisiko deleted the detox/add-assertion-class branch October 18, 2023 17:54
@github-actions github-actions bot locked and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
E2E No QA Needed/E2E Only Apply this label when your PR does not need any QA effort. release-7.10.0 team-mobile-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants