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

improve bookmarks and favorites UI tests #3019

Merged
merged 17 commits into from
Jul 3, 2024
Merged

Conversation

brindy
Copy link
Contributor

@brindy brindy commented Jul 1, 2024

Task/Issue URL: https://app.asana.com/0/392891325557410/1207195241923822/f
Tech Design URL:
CC: @bwaresiak

Description:
Fleshed out and tweaked bookmarks and favorites UI tests

Notes:

  • The scripts are being reviewed in scripts for running UI tests #3000 so you can largely ignore them.
  • You should be able to run the scripts from anywhere, just adjust the paths below accordingly
  • Fixes a bug found during testing in the bookmarks swift code

Steps to test this PR:

  • Confirm End to End tests have passed: https://github.com/duckduckgo/iOS/actions/runs/9750711656
  • In terminal cd into .maestro
  • Run setup_ui_tests.sh
  • Optional: run all the release tests in one go using: run_tests_tests.sh release_tests
  • Run run_ui_tests.sh release_tests/bookmarks.yaml
  • Run run_ui_tests.sh release_tests/bookmarks-multi.yaml
  • Run run_ui_tests.sh release_tests/favorites.yaml

@brindy brindy changed the title bookmarks UI tests improve bookmarks and favorites UI tests Jul 1, 2024
@brindy brindy requested a review from loremattei July 1, 2024 17:10
Copy link

github-actions bot commented Jul 1, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 53bd61b

@@ -432,7 +432,7 @@
<scene sceneID="gmt-6C-Bi5">
<objects>
<tableViewController storyboardIdentifier="BookmarkFoldersViewController" id="o1q-bZ-7v3" userLabel="Bookmark Folders" customClass="BookmarkFoldersViewController" customModule="DuckDuckGo" customModuleProvider="target" sceneMemberID="viewController">
<tableView key="view" clipsSubviews="YES" contentMode="scaleToFill" alwaysBounceVertical="YES" dataMode="prototypes" style="insetGrouped" allowsSelectionDuringEditing="YES" rowHeight="-1" estimatedRowHeight="-1" sectionHeaderHeight="18" sectionFooterHeight="18" id="V1z-bn-1Gk">
<tableView key="view" clipsSubviews="YES" contentMode="scaleToFill" alwaysBounceVertical="YES" keyboardDismissMode="onDrag" dataMode="prototypes" style="insetGrouped" allowsSelectionDuringEditing="YES" rowHeight="-1" estimatedRowHeight="-1" sectionHeaderHeight="18" sectionFooterHeight="18" id="V1z-bn-1Gk">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - without it, on some screens when you do

  • assertVisible: "Delete"
  • tapOn: "Delete"

Then maestro thinks that has worked when it hasn't and scrolling on that page doesn't bring the delete button up fair enough. The usual practice is to dismiss the keyboard on swiping a list anyway so I think it's OK.

@@ -444,6 +438,8 @@ class BookmarksViewController: UIViewController, UITableViewDelegate {
completion(true)
}
present(alertController, animated: true)
} else if bookmark.isFolder {
delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is to avoid the debug assertion, right?
Why aren't we calling the completion handler here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll check that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a few paths that don't but I think this one should so I've added it. I had a quick look and can't really see what the implication of not calling it is. It seems that if you don't then the buttons shown on swipe don't get hidden, but since we're removing the row anyway it doesn't matter too much.

Copy link
Contributor

@loremattei loremattei left a comment

Choose a reason for hiding this comment

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

@brindy LGTM!
I left a couple small questions just to double check, but feel free to merge if things look good to you.

@brindy brindy merged commit 37d9f2f into main Jul 3, 2024
14 checks passed
@brindy brindy deleted the brindy/bookmarks-ui-tests branch July 3, 2024 14:45
samsymons added a commit that referenced this pull request Jul 4, 2024
* main: (24 commits)
  Add pixels to measure baseline for New Tab Page sections (#3024)
  backgrounding UI tests (#3021)
  improve bookmarks and favorites UI tests (#3019)
  [DuckPlayer] - 4. Remote Config (#3018)
  Updates BSK
  Fix issue when cancelling a download (#3030)
  Tentative fix for a crash and connectivity issues (#3027)
  autofill UI tests (#3012)
  scripts for running UI tests (#3000)
  Integrate RemoteMessaging with NewTabPage (#3017)
  Update to iOS 15 deployment target (#3001)
  Fire a pixel when removing the VPN configuration (#3014)
  Release 7.127.0-0 (#3020)
  point to BSK branch (#3016)
  [DuckPlayer] - 3. URL management & FE comms (#3007)
  Bump BSK version (#3011)
  New Tab Page layout and base elements (#3008)
  Remove Privacy Pro from device once expired account is deleted (#3009)
  Improvements to subscription settings (#2959)
  Toggle reports limiter (#3005)
  ...
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.

None yet

2 participants