UI Tests: Bookmarks and Favorites tests #2568
Conversation
…n English phrase and update in test
# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # UITests/BookmarksBarTests.swift
| self.setAccessibilityValue(accessibilityValue) | ||
| return self | ||
| } | ||
|
|
||
| @discardableResult |
There was a problem hiding this comment.
Set accessibility values on menu items.
| @@ -24,7 +24,7 @@ enum UITests { | |||
| /// Timeout constants for different test requirements | |||
| enum Timeouts { | |||
| /// Mostly, we use timeouts to wait for element existence. This is about 3x longer than needed, for CI resilience | |||
| static let elementExistence: Double = 2.5 | |||
| static let elementExistence: Double = 5.0 | |||
| /// The fire animation time has environmental dependencies, so we want to wait for completion so we don't try to type into it | |||
| @@ -55,7 +55,6 @@ class BookmarksBarTests: XCTestCase { | |||
| resetBookmarksAndAddOneBookmark() | |||
| app.typeKey("w", modifierFlags: [.command, .option, .shift]) // Close windows | |||
| openSettingsAndSetShowBookmarksBarToUnchecked() | |||
| settingsWindow = app.windows.containing(.checkBox, identifier: "Preferences.AppearanceView.showBookmarksBarPreferenceToggle").firstMatch | |||
| openSecondWindowAndVisitSite() | |||
There was a problem hiding this comment.
Remove unused XCUIElement.
| "\(self.debugDescription) didn't load with the expected title in a reasonable timeframe." | ||
| ) | ||
| self.hover() | ||
| } | ||
| } |
There was a problem hiding this comment.
The Bookmarks and Favorites text case is a large test case which contains several individual tests which have enough steps that it becomes difficult to reason about their chain of events without some abstraction. Click/hover after proving the existence of an XCUIElement seems like the easiest semantic improvement which doesn't hide steps which increase understanding of what an individual test does that is different from the other tests. If you agree, I'm happy to apply this abstraction to the previous test cases in a separate pull request.
There was a problem hiding this comment.
Click/Hover after asserting existence sounds good to me! I agree that it would be beneficial to apply this to other tests, at your convenience. Thanks!
| private var showBookmarksBarAlways: XCUIElement! | ||
| private var showBookmarksBarPopup: XCUIElement! | ||
| private var showFavoritesPreferenceToggle: XCUIElement! | ||
|
|
There was a problem hiding this comment.
This is a big test case.
| let deleteContextMenuItemCoordinate = bookmarkBarBookmarkIcon.coordinate(withNormalizedOffset: CGVector(dx: 0.9, dy: 9.0)) | ||
| bookmarkBarBookmarkIconCoordinate.rightClick() | ||
| deleteContextMenuItemCoordinate.click() | ||
| app.typeKey("w", modifierFlags: [.command, .option, .shift]) |
There was a problem hiding this comment.
This is a fragile test. The reason I have included a fragile test here is because the current design of the bookmark item on the bookmark bar is not easy to give accessibility consciousness to, since it isn't an accessibility type of interface element and it wasn't clear to me how it receives the responder chain. This meant that I didn't really have what I felt was a safe anchor point at which I could add the pointers to accessibility ID, role, enabled status and values as I can usually do for other interface which isn't yet accessibility-enabled. In this case I felt that constructing this anchor point was likely to do something more problematic than writing a fragile test, which would be to run the risk of changing the actual behavior while attempting to refactor. This one test also didn't really seem like an appropriate launching point for a discussion about changing this interface's design, since it is road-tested, and that is more valuable than an individual test.
What this means in practice is that, because the parent element under test is not enabled, its descendants are never enabled, so starting with the right-click and ending with the dismissal of the context menu, the items under test are addressed by their coordinates rather than their identifiers or values. It's fragile to do this because the coordinates can change and they are also likely to be different for users who are using accessibility accommodations which change the scale of UI elements.
My suggestion for this test is to keep it for now because it should be able to prove that this interface works in CI and most local setups, but when the bookmark element on the bookmark bar comes up for SwiftUI implementation or other modernization, revisit this test and operate it the right way, according to accessibility ID. The other tests in this test case do not have this issue.
There was a problem hiding this comment.
Thanks for the explanation @Halle. I think it's fine and we have to accept it for now. Could you please add a short comment in the code about this, please? Just for future reference. Thanks a lot!
There was a problem hiding this comment.
Great, I added a comment that says basically the same thing, which should make it clear when it's a good time to update it (and which part to update).
There was a problem hiding this comment.
LGTM, great job @Halle! Tests are consistently passing for me if only I have my second monitor disconnected, which I think is a reasonable trade-off that we can document somewhere under "instructions on running UI tests locally". Thanks!
| "\(self.debugDescription) didn't load with the expected title in a reasonable timeframe." | ||
| ) | ||
| self.hover() | ||
| } | ||
| } |
There was a problem hiding this comment.
Click/Hover after asserting existence sounds good to me! I agree that it would be beneficial to apply this to other tests, at your convenience. Thanks!
Task/Issue URL: https://app.asana.com/0/1199230911884351/1205717021705367/f
Tech Design URL:
CC:
Description: Adds a series of UI tests for Bookmarks and Favorites
Steps to test this PR:
Note: If this builds a
reviewbuild that is treated as a first run on your system during every test (for instance, asking you where to put the application), please build and run the scheme once instead of running its tests, and go through the new app install first run steps once before running the tests, and answer any first install questions.UI Tests general guidelines for everyone: it unfortunately isn't possible to multitask while running UI tests on your local machine, because you will change or intercept screen interactions that the tests depend on. If you experience failures in your local environment, please always send the
xcresultbundle so it is possible to view what happened differently on your machine, even if it seems like the same failure as a previous failure (it may be subtly different). Since thexcresultbundle will include a screen recording, for your privacy, please consider things like your chats, calls, and open documents that you don't want to send in a screen recording when you are running the tests. Thank you very much for your help!