Task/jenkins 36641 smart web element improvements #1529

cliffmeyers · 2017-11-03T20:27:19Z

Description

While trying to write more comprehensive tests for favorites, I found some limitations in SmartWebElement and WebDriverMixin API's (see immediately below):

When retrieving a SmartWebElement, chained calls to findElement/s method would use raw Selenium API's that don't use the polling/wait behavior making them very subject to timing issues. We definitely want to be able to use these API's so we can write a simple selector for a top level element, and then do searches within that element (keeping individual selector expressions simple).
The WebDriverMixin.find API always returned a single SmartWebElement which might actually correspond to multiple elements. There are times you want to interact with multiple elements, and the rest of WebDriver API's use a List<WebElement> in many, so it doesn't make sense IMO to conflate "one element" and "many elements" into SmartWebElement abstraction.

My attempt to improve these are described by these changes:

Ensure that SmartWebElement.findElement/s (1) uses waits internally when finding descendent elements, and (2) returns SmartWebElements, otherwise chaining calls won't work well since we'll lose implicit wait behavior.
Remove getElement/s since it overlapped with findElements/s and was only visible on SmartWebElement class... ideally we can just code against WebElement interface.
Change WebDriverMixin.find API to differentiate between find/findMany so that client code can work with single element or list of elements
Add WebDrivenMixin.findNow so we have ability to do queries that don't use an implicit wait, so we can check that elements aren't present
Add WebDrivenMixin.untilCondition we can leverage full power of ExpectedConditons.* utility methods. These are powerful and we should offer full-class support.

PLEASE NOTE Still some internal cleanup in this PR that needs to happen but I think it's getting close. Please note the TODOs.

Submitter checklist

Link to JIRA ticket in description, if appropriate.
Change is code complete and matches issue description
Appropriate unit or acceptance tests or explanation to why this change has no tests
Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

Run the changes and verified the change matches the issue description
Reviewed the code
Verified that the appropriate tests have been written or valid explanation given

…in the WebDriver instance and passing it around; add a couple utility methods for related to waits and element wrapping

…y when finding descendent elements, and (2) returns SmartWebElements, otherwise chaining calls won't work well since we'll lose implict wait behavior B. remove getElement/s since it overlapped with findElements/s and was only visible on SmartWebElement class C. change WebDriverMixin API to differentiate between find/findMany so that client code can work with single element or list of elements D. add WebDrivenMixin.findNow so we have ability to do queries that don't use an implicit wait, so we can check that elements aren't present E. add WebDrivenMinx.untilCondition we can leverage full power of ExpectedConditons.* utilities

…ardPage" to exercise some of the new APIs

cliffmeyers · 2017-11-03T20:29:15Z

acceptance-tests/src/main/java/io/blueocean/ath/LocalDriver.java

- * Accepts expressions for css and xpath, if the provided lookup starts with a /, XPath is used
+ * Provides access to the current WebDriver instance.
+ * Contains some utility methods for working with waits and SmartWebElement.
+ * TODO: rename this class as it's *not* a WebDriver but rather more of a DriverHolder.


All of the client code was calling LocalDriver.getDriver() which returned the thread local'd RemoteWebDriver. No need for LocalDriver to impl WebDriver as far as I can see.

cliffmeyers · 2017-11-03T20:30:39Z

acceptance-tests/src/main/java/io/blueocean/ath/LogEntryLogger.java

+        } else {
+            logger.info(String.format("nothing useful written to browser console; %s entries were hidden", entries.size()));
+        }
+    }


Overall these changes make the browser logging stuff a bit more explicit about when 1. nothing is logged, 2. nothing is logged but useless messages, and 3. there are actually useful messages.

cliffmeyers · 2017-11-03T20:32:49Z

acceptance-tests/src/main/java/io/blueocean/ath/SmartWebElement.java

    }

    @Override
    public void click() {
        for (int i = 0; i < RETRY_COUNT; i++) {
            try {
-                WebElement e = getElement();
+                WebElement e = buildWait().until(ExpectedConditions.elementToBeClickable(element));


This got lost in #1484 and needs to be there to make sure we don't click on elements that are transitioning from disabled to enabled. Frequent cause of flakiness in creation tests when robot picks a repo and clicks "Create Pipeline" while it's still disabled, resulting in a hung test.

interesting

right, definitely need this change after talking with Cliff, this was a stupid mistake on my part

cliffmeyers · 2017-11-03T20:33:11Z

acceptance-tests/src/main/java/io/blueocean/ath/SmartWebElement.java

-        return (T)((JavascriptExecutor)getDriver()).executeScript(script, el, elements);
+        // TODO: validate this change is correct
+        WebElement el = getElement();
+        return (T)((JavascriptExecutor)getDriver()).executeScript(script, el);


@kzantow perhaps you can weigh in whether this change is appropriate.

why change this?

elements no longer exists

Can you let me know if this change seems ok in light of the fact that there is no List from getElements() to pass down into executeScript ?

@kzantow can you let me know if you think this change is okay given that getElements() no longer exists, therefore passing the elements array as the last arg isn't possible anymore

I also don't think removing getElements() is necessary or the right change. Again, making getElements() support a parent SmartWebElement will ensure that everything is always operating on the current state of the page when it's called.

cliffmeyers · 2017-11-03T20:35:32Z

acceptance-tests/src/main/java/io/blueocean/ath/SmartWebElement.java

+        return buildWait().until((WebDriver driver) -> {
+            WebElement matchedElement = getElement().findElement(locator);
+            return LocalDriver.wrapElement(getDriver(), matchedElement, locator);
+        });


Please have a look at this code in particular to make sure it makes sense.

cliffmeyers · 2017-11-03T20:37:16Z

acceptance-tests/src/main/java/io/blueocean/ath/SmartWebElement.java

    }

    @Override
    public boolean isDisplayed() {
-        WebElement e = getElement();
+        WebElement e = buildWait().until(ExpectedConditions.visibilityOf(element));
        return e.isDisplayed();


I think in its current state this method can only ever return true or throw an exception. I might be wrong. Not sure if this should have some error handling baked in, or maybe left alone.

michaelneale · 2017-11-03T22:48:29Z

Looks interesting to me... cc @imeredith maybe take a look on monday?

kzantow · 2017-11-06T17:28:53Z

acceptance-tests/src/main/java/io/blueocean/ath/SmartWebElement.java

    public static final int RETRY_COUNT = 3;

    private WebDriver driver;
+    protected WebElement element;


So this I strongly discourage. Part of the thing is that if you have a SmartWebElement all operations happen when you invoke them, this avoids any cases where you have a stale reference to a WebElement for some reason

Let me see if I can reintroduce that lazy logic so we don't hold a ref to any WebElement.

Now I finally understand the motivation, so you can wrap the other returned WebElements with this same logic. Ok, let me stew on this for just a little while

Right. Otherwise can't really chain findElement calls together because we'll be returning a mix of SmartWebElement and RemoteWebElement which I think would lead to a lot of flake since some WebElement's have wait behavior baked in and others do not.

The alternative with original code is that we'd need to do all our finds through WebDriverMixin.find to ensure we always get a SWE, which could lead to really lengthy selectors:

find(".favorites-card-stack .pipeline-card[data-fullname="cliffs-pipeline"] .Checkbox.Favorite > label").click()

rather than

find(".favorites-card-stack .pipeline-card[data-fullname="cliffs-pipeline"] .findElement(".Checkbox.Favorite > label") .click()

I find the latter really useful because we can write individual methods on our page objects to grab specific elements, and we can compose those methods just by chaining the findElement calls.

I wonder if we could add some exception handling into SWE for StaleElementException that retries the locator. It wouldn't necessarily work for elements fetched via findMany since the expression stored to fetch them would match multiple elements. I can't say I've run into any StaleElementException yet but that's part of why I am working on a branch derived from this one to add really thorough tests to personalization as a testing ground.

kzantow · 2017-11-06T22:16:46Z

acceptance-tests/src/main/java/io/blueocean/ath/LocalDriver.java

     */
-    public static void use(WebDriver driver, Procedure proc) {


I would not remove this, it can be useful to write tests that rely on multiple windows

... still, I would not remove this

kzantow · 2017-11-06T22:20:21Z

acceptance-tests/src/main/java/io/blueocean/ath/SmartWebElement.java

-     * Iterates over all found elements with the given function
-     * @param fn to perform on the elements
-     */
-    public void forEach(java.util.function.Consumer<WebElement> fn) {


So this provided that way to operate on a collection of elements. You mentioned there are cases where we need to pass the list of the base WebElement to webdriver functions, are there examples where we're doing that?

kzantow · 2017-11-06T22:22:45Z

acceptance-tests/src/main/java/io/blueocean/ath/SmartWebElement.java

-     * Determines if the element is visible
-     * @return true if visible, false if not
-     */
-    public boolean isVisible() {


i guess this is just duplicate noise, but i really don't like isDisplayed this mirrors jQuery's terminology

…ge this over to SmartWebElement stuff once #1529 lands

…ge this over to SmartWebElement stuff once #1529 lands (#1544)

…vements

cliffmeyers · 2017-11-14T21:44:09Z

@kzantow I've made some updates to this PR. Based on our past conversations, can you let me know if you think these changes are suitable or should I continue to iterate? Perhaps we can touch base in a meeting if that's easier to discuss.

…doned PR #1529

cliffmeyers · 2017-11-15T19:30:06Z

@kzantow indicated he wants to add some similar features as in this PR but with less code, so I am closing this PR. I will carry over the improvements to browser console logging in another PR.

* improve the display of log entries from browser console * extract resource resolving logic into standalone class so we don't dictate the ATH test superclass * fix a bug where ClassicJobApi.createPipeline would not actually respect the parent folder passed in * add "create" method to FreestyleJob * add dependencies for HttpRequest * add utilities for working with the "fully qualified name" for a pipeline so its favorite card can be located * add a new "FreestyleFavoritesTest" and corresponding "FavoritesDashboardPage" to exercise some of the new APIs * add some tests that verify favorite cards can be added and removed for three primary job types * fix public visibility issue * ensure FavoritesAddRemoveTest cleans up any jobs after it finishes running * remove test case that was mostly for testing purposes * rework FavoritesAddRemoveTest to not rely on API's introduced in abandoned PR #1529 * improve the docs for SSEEvents.activityComplete * add a method to build a Freestyle job * add a helper method to navigate back * testing favorites navigation for freestyle and classic * add more browser navigation utilities * add a utility to serve as an adapter between WebElement and By locators * add extra clickability check for "Branches" tab * comments * add test cases for a multibranch pipeline * refactor page object to appropriate source folder / package * add an enum that contains different "status" values used in UI (flattening of "state" and "result" from BlueRun enums) * add tests against the favorite card actions (freestyle, classic pipeline, multibranch) * scan class hierarchy for @Login annotation so that it can be declared on a superclass * refactor duplicated code into "AbstractFavoritesTest" * remove empty file * fix a flakiness issue observed in FavoritesCardsTest.testMultibranch where the favorite card would sometimes update to white color due to receiving a status of "finished" which would fail the test. soemtimes SSE is received for job_run_ended event where state=FINISHED and result=UNKNOWN which is nonsense. must be some race condition where properties are not mutated together. long term we need a cleaner solution where a run fetched in an inconsistent state is simply refetched

Cliff Meyers added 4 commits November 1, 2017 16:36

improve the display of log entries from browser console

90d65a2

LocalDriver doesn't need to implement WebDriver as it's just holding …

7b285cc

…in the WebDriver instance and passing it around; add a couple utility methods for related to waits and element wrapping

add a new "FreestyleFavoritesTest" and corresponding "FavoritesDashbo…

f1b7039

…ardPage" to exercise some of the new APIs

cliffmeyers added the work-in-progress label Nov 3, 2017

cliffmeyers requested review from kzantow, michaelneale and imeredith November 3, 2017 20:27

cliffmeyers commented Nov 3, 2017

View reviewed changes

kzantow reviewed Nov 6, 2017

View reviewed changes

cliffmeyers mentioned this pull request Nov 7, 2017

JENKINS-45393: Bitbucket server ATH #1492

Merged

cliffmeyers pushed a commit that referenced this pull request Nov 9, 2017

use wait.click to deflake the GithubEditorTest; eventually we'll chan…

fb546f2

…ge this over to SmartWebElement stuff once #1529 lands

cliffmeyers mentioned this pull request Nov 9, 2017

hotfix / github editor test deflake #1544

Merged

7 tasks

cliffmeyers added a commit that referenced this pull request Nov 10, 2017

use wait.click to deflake the GithubEditorTest; eventually we'll chan…

5543861

…ge this over to SmartWebElement stuff once #1529 lands (#1544)

Cliff Meyers added 2 commits November 14, 2017 11:18

Merge branch 'master' into task/JENKINS-36641-smart-web-element-impro…

7134e1c

…vements

wrap the result of WebDriverMixin.untilCondition in SmartWebElement

b688e25

cliffmeyers added needs-review and removed work-in-progress labels Nov 14, 2017

cliffmeyers pushed a commit that referenced this pull request Nov 15, 2017

rework FavoritesAddRemoveTest to not rely on API's introduced in aban…

6e8b17d

…doned PR #1529

cliffmeyers closed this Nov 15, 2017

halkeye deleted the task/JENKINS-36641-smart-web-element-improvements branch January 24, 2019 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task/jenkins 36641 smart web element improvements #1529

Task/jenkins 36641 smart web element improvements #1529

cliffmeyers commented Nov 3, 2017 •

edited

Loading

cliffmeyers Nov 3, 2017

cliffmeyers Nov 3, 2017

cliffmeyers Nov 3, 2017

michaelneale Nov 3, 2017

kzantow Nov 6, 2017

cliffmeyers Nov 3, 2017

kzantow Nov 6, 2017

cliffmeyers Nov 7, 2017

cliffmeyers Nov 14, 2017

cliffmeyers Nov 14, 2017

kzantow Nov 15, 2017

cliffmeyers Nov 3, 2017

cliffmeyers Nov 3, 2017

michaelneale commented Nov 3, 2017

kzantow Nov 6, 2017

cliffmeyers Nov 6, 2017

kzantow Nov 7, 2017

cliffmeyers Nov 7, 2017

kzantow Nov 6, 2017

kzantow Nov 15, 2017

kzantow Nov 6, 2017

kzantow Nov 6, 2017

cliffmeyers commented Nov 14, 2017

cliffmeyers commented Nov 15, 2017

		*/
		public static void use(WebDriver driver, Procedure proc) {

Task/jenkins 36641 smart web element improvements #1529

Task/jenkins 36641 smart web element improvements #1529

Conversation

cliffmeyers commented Nov 3, 2017 • edited Loading

Description

Submitter checklist

Reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelneale commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffmeyers commented Nov 14, 2017

cliffmeyers commented Nov 15, 2017

cliffmeyers commented Nov 3, 2017 •

edited

Loading