[ATH IMPROVEMENT] Allow local Jenkins instance, simplified API #1484

kzantow · 2017-10-12T06:46:11Z

Adding ability to run the ATH locally against a locally running dev instance.

kzantow · 2017-10-12T06:46:34Z

acceptance-tests/src/main/java/io/blueocean/ath/sse/SSEClientRule.java

@@ -93,7 +94,7 @@ public void setLogEvents(boolean logEvents) {
    public void connect() throws UnirestException, InterruptedException {
        SecureRandom rnd = new SecureRandom();
        String clientId = "ath-" + rnd.nextLong();
-        HttpResponse<JsonNode> httpResponse = Unirest.get(baseUrl + "/sse-gateway/connect?clientId=" + clientId).asJson();
+        HttpResponse<JsonNode> httpResponse = Unirest.get(baseUrl + "/sse-gateway/connect?clientId=" + clientId).basicAuth(LoginPage.getUsername(), LoginPage.getPassword()).asJson();


might need to add some logic when to do this

if running unauthed I guess it may ignore those basic headers anyway (maybe...)

We can set the logged in user in the state fairly easily (when using @Login)

kzantow · 2017-10-12T06:49:29Z

acceptance-tests/src/main/java/io/blueocean/ath/pages/classic/LoginPage.java

-            return ExpectedConditions.urlToBe(base + "/").apply(driver);
-        });
-        logger.info("Logged in as alice");
+        find("input[name=j_username]").sendKeys(getUsername());


This was a 30 minute example of how my prior testing frameworks looked... well, similar.

Only thing I would put here is that I would make find use "By" class. Ie find(By.id(..)), good otherwise. Could default to use css for strings though

I suppose it doesn't hurt to have an override, but I would definitely not use By as the only way to specify things since you can specify a find by id using css like #my-fancy-id, which would work with this already. The only thing missing is find by link text, which you can use xpath for, but is a little more cumbersome. We really shouldn't be finding anything by text in these tests, anyway, tho

michaelneale · 2017-10-12T07:01:21Z

nice

kzantow · 2017-10-12T07:03:07Z

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

@@ -27,7 +27,7 @@ public static void clearText(WebElement element) {
     */
    public static void setText(WebElement element, String text) {
        checkTextElement(element);
-        element.sendKeys(Keys.chord(Keys.CONTROL, "a"));
+        element.clear();//sendKeys(Keys.chord(Keys.CONTROL, "a"));


is there a reason it was doing CTRL-a instead of clear before? this definitely doesn't work on Mac ;)

ctrl-a goes back to the start of a text cell (emacs keys - may they never die) which I gather is not the intent here?

Sorry, somehow I completely missed the presence of clear() in the docs. That is much easier. :)

cliffmeyers · 2017-10-12T16:18:54Z

acceptance-tests/src/main/java/io/blueocean/ath/pages/classic/LoginPage.java

-        logger.info("Logged in as alice");
+        find("input[name=j_username]").sendKeys(getUsername());
+        find("input[name=j_password]").sendKeys(getPassword());
+        find("//button[contains(text(), 'log')]").click();


I like this shorthand overall. Per discussion in Gitter, I think we want a static click method that implements the same retry logic as here: https://github.com/jenkinsci/blueocean-plugin/pull/1412/files#diff-9d7946f8ccd8a4191146940351b51999

Yeah, absolutely. This is why I said it was a 30 minute implementation. The real logic typically ends up being more complex, and yeah, definitely agree what we do is add utility methods like click with retry and setText which should be doing what sendKeys is doing now, e.g. you just want to set the text of an input, but you may want to send individual keys for other purposes e.g. testing an autocomplete

cliffmeyers · 2017-10-12T16:24:36Z

I like where this PR is headed although I would like to land #1412 first. We'd also need to check to make sure that the changes to make ATH run better in Chrome do not fail in Firefox - perhaps we can do that in this PR?

Down the road we probably want to take a look at Selenium Grid for running multiple browsers, maybe as part of the weekly CI builds we do now? https://github.com/SeleniumHQ/selenium/wiki/Grid2

kzantow · 2017-10-12T16:35:10Z

@cliffmeyers yes, this is just a bit of an example (and something working that makes it easier to run tests locally - e.g. no need for docker, etc.). Certainly land #1412 first and then look to revisit this and combine the changes in a sensible way moving forward.

imeredith · 2017-10-16T03:05:18Z

I like what is going on here.

kzantow · 2017-10-17T05:46:46Z

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

+/**
+ * Provides utility methods to downstream test classes
+ */
+public class BasePage {


This should be named something else...

Perhaps BasePageObject or AbstractPageObject and made abstract if not meant to be instantiated directly.

# Conflicts: # acceptance-tests/src/main/java/io/blueocean/ath/AthModule.java

cliffmeyers

Left some feedback that hopefully spurs further discussion.

We definitely want to work that click retry logic into the appropriate spot. Then I think shift our API usage to click(".some-css-classname") rather than find(".some-css-classname").click() unless it makes sense to bake the retry impl into the WebElement itself, in which case "trailing click" usage would be fine? Without having done this myself or checked out the code and debugged through it I'm having trouble knowing what's the correct route to go.

cliffmeyers · 2017-10-18T19:19:27Z

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

-import io.blueocean.ath.factory.ClassicPipelineFactory;
-import io.blueocean.ath.factory.RunDetailsArtifactsPageFactory;
-import io.blueocean.ath.factory.RunDetailsPipelinePageFactory;
+import io.blueocean.ath.factory.*;


I seem to recall that using wildcard imports violates some style rules in Jenkins projects, I think @vivek pointed that out to me once. If you're using IDEA and search for "class count to use import with" in settings, you'll find an option. I set it to 100.

cliffmeyers · 2017-10-18T19:21:19Z

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

+/**
+ * Provides utility methods to downstream test classes
+ */
+public class BasePage {


Perhaps BasePageObject or AbstractPageObject and made abstract if not meant to be instantiated directly.

cliffmeyers · 2017-10-18T19:22:33Z

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

+     * Navigates to a relative url to the base url
+     * @param url where to go
+     */
+    public void go(String url) {


I like the idea of two methods to navigate to relative and absolute paths, although it's not clear which one is which from the names go and goTo

agreed, maybe could just inspect the url if it starts with http(s) then don't use the base

cliffmeyers · 2017-10-18T19:37:22Z

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

+    }
+
+    @Override
+    public void submit() {


We don't really have forms that truly submit in the app, should we just remove this?

it's necessary to implement the interface

cliffmeyers · 2017-10-18T19:45:15Z

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

+
+    @Override
+    public void click() {
+        forEach(e -> e.click());


Do want to support clicking multiple elements? It seems that it could have unintended consequences if the selector is too loose. Perhaps warn if multiple elements are matched?

We should bake in the retry logic from WaitUtil.click here I believe.

cliffmeyers · 2017-10-18T19:45:37Z

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

+
+    @Override
+    public void sendKeys(CharSequence... charSequences) {
+        forEach(e -> e.sendKeys(charSequences));


Same question as in click

yeah, probably not, might be good to dump a warning for those; i'll make those changes - or throw an error?

cliffmeyers · 2017-10-18T19:50:35Z

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

+    }
+
+    @Override
+    public boolean isEnabled() {


Would this style of method implicitly wait for the element to be enabled, or do an immediate check for whether it's enabled? If the latter, it seems like it could be subject to timing failures. Is there logic we can borrow from ExpectedConditions here? I honestly don't know the answer but the stuff in WaitUtil generally seems to wait the right way, although maybe the API can be improved here?

just checks for it being enabled. sometimes you might want to check for things enabled/disabled based on user input. could add more methods as needed e.g. waitUntilEnabled

cliffmeyers · 2017-10-18T19:52:37Z

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

+    }
+
+    @Override
+    public String getText() {


Similar question as above, say for example the textToBePresentInElement method on ExpectedConditions seems useful here. Again I'm a little foggy on these API's as to whether that's appropriate in the WebElement impl or if we need a util method on BasePage instead? Sorry if I'm muddying the waters.

i really hope we can't find an element without text while in the background somehow it gets filled in. that kind of behavior would be a bit ridiculous, even for react. these types of things we can at least identify and bake in to this API as needed instead of cluttering up the tests with all kinds of wait conditions

michaelneale · 2017-10-22T21:50:05Z

I think this is ok if no one else has objections, not sure what the details are though - perhaps README could be updated?

michaelneale · 2017-10-23T01:36:40Z

acceptance-tests/src/main/java/io/blueocean/ath/pages/blue/GithubCreationPage.java

@@ -105,7 +103,7 @@ public void selectPipelineToCreate(String pipeline){
    }

    public void clickCreatePipelineButton() {
-        wait.until(ExpectedConditions.elementToBeClickable(createPipelineButton)).click();
+        click(".button-create");


michaelneale · 2017-10-23T01:52:52Z

FWIW: #1503 seemed to pass (fixing the click/co-ordinates problem) but will leave that one in favour of this if this one is better and works.

michaelneale · 2017-10-23T03:28:48Z

I was trying this locally - testEditor fails for me (times out) - is @cliffmeyers or @kzantow able to run it locally? (master + https://github.com/jenkinsci/blueocean-plugin/pull/1503/files does work for me though). Not sure if intermittent or something else, but I am wary of changes if I can't run them locally

EDIT: after a few tries it is ok (my machine struggles) - so I think this may be LGTM

I think @cliffmeyers is ok with it? woudl be good to get master stable @kzantow

michaelneale · 2017-10-23T03:32:41Z

yep 🐝 from me.

I had too many things running...

sophistifunk · 2017-10-23T03:34:36Z

Is this the fix for the missing button?

kzantow · 2017-10-23T03:35:16Z

@sophistifunk this is a number of improvements to ATH and that fix, yes

sophistifunk · 2017-10-23T03:43:24Z

Sweet, when it's merged I'll pull it into my branch in the quest for fields verdant.

michaelneale · 2017-10-23T05:18:45Z

@sophistifunk yes this will fix it

cliffmeyers · 2017-10-23T13:27:19Z

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

+    public WebElement getElement() throws NoSuchElementException {
+        List<WebElement> elements = getElements();
+        if (elements == null || elements.isEmpty()) {
+            throw new NoSuchElementException("Nothing matched to click on for: " + expr);


The reference to "click" here might be copy/paste error?

ah yes, will fix; oops

cliffmeyers · 2017-10-23T13:29:17Z

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

+                    logger.debug("exception: " + ex.getMessage());
+                    try {
+                        // typically this is during an animation, which should not take long
+                        Thread.sleep(500);


I had assumed I'd need to add a sleep to this method, although it worked very reliably without it (at least in my local env). I suppose there's no harm in being more conservative by having it pause longer before retry.

Well, what I'd like to do (and maybe what I did before, I don't quite remember) is get rid of the WebDriver wait and replace it with some smart polling / retries of our own. It should speed things up in some cases. We'll want to add some other utilities to try to have some explicit waits for a css selector, for example waiting for github to scan repos and such, we know that will take a while.

cliffmeyers · 2017-10-23T13:44:57Z

...ipeline-api-impl/src/test/java/io/jenkins/blueocean/rest/impl/pipeline/PipelineNodeTest.java

@@ -2031,6 +2032,21 @@ public void testBlockedStep() throws Exception {
                assertEquals(1, stepsResp.size());
                assertEquals("QUEUED", stepsResp.get(0).get("state"));
            }
+        } else {
+            // Avoid spurious code coverage failures


Nice, had these fail from time to time myself... 👍

cliffmeyers

Change all LGTM 🐝

kzantow · 2017-10-23T13:53:06Z

... merging once green :)

kzantow · 2017-10-23T14:58:01Z

Last 3 runs all green, I think this fixes the latest flakiness on master.

Update ATH tests for new flow

87ecc77

kzantow added the work-in-progress label Oct 12, 2017

kzantow commented Oct 12, 2017

View reviewed changes

Handle more close cases for errors

33ccc9c

kzantow changed the title ~~[WiP] Allow local ATH run/dev~~ [WiP] Allow local Jenkins instance with ATH run/dev Oct 12, 2017

kzantow commented Oct 12, 2017

View reviewed changes

cliffmeyers reviewed Oct 12, 2017

View reviewed changes

Some refactoring

f8f224d

kzantow added 2 commits October 17, 2017 01:12

Add ability to read a properties file for ATH (~/.blueocean-ath-config)

f070bbe

Add ability to read a properties file for ATH (~/.blueocean-ath-config)

78aa546

kzantow commented Oct 17, 2017

View reviewed changes

kzantow added 7 commits October 17, 2017 02:01

Add ability to read a properties file for ATH (~/.blueocean-ath-config)

f605005

Whoops

8d010e5

Whoops, whoops

1c58498

Merge remote-tracking branch 'primary/master' into ATH-allow-local-dev

a17c707

Whoops, whoops

97e6cf4

Missed a couple things

04bb80f

Merge remote-tracking branch 'primary/master' into ATH-allow-local-dev

cdd1ed4

# Conflicts: # acceptance-tests/src/main/java/io/blueocean/ath/AthModule.java

cliffmeyers reviewed Oct 18, 2017

View reviewed changes

kzantow added 6 commits October 18, 2017 18:28

Tweaks based on PR feedback

972f4a0

Consolidate go method

d89172d

Consolidate go method

2201f7a

Imports

5cce6ec

Merge remote-tracking branch 'primary/master' into ATH-allow-local-dev

4cb6779

Imports

aa434f2

Merge remote-tracking branch 'primary/master' into ATH-allow-local-dev

c686b05

kzantow added needs-review and removed work-in-progress labels Oct 21, 2017

kzantow changed the title ~~[WiP] Allow local Jenkins instance with ATH run/dev~~ [ATH IMPROVEMENT] Allow local Jenkins instance, simplified API Oct 21, 2017

Use 'improved' click logic for github pipeline create

fced2e0

michaelneale reviewed Oct 23, 2017

View reviewed changes

michaelneale mentioned this pull request Oct 23, 2017

Add (inactive) support for auto-formatting on commit #1501

Merged

michaelneale mentioned this pull request Oct 23, 2017

JENKINS-47378# Switch to JDK8 and core 2.73.2 #1479

Merged

7 tasks

Merge remote-tracking branch 'primary/master' into ATH-allow-local-dev

9922950

cliffmeyers reviewed Oct 23, 2017

View reviewed changes

Typo

add4f4a

cliffmeyers reviewed Oct 23, 2017

View reviewed changes

cliffmeyers approved these changes Oct 23, 2017

View reviewed changes

Merge remote-tracking branch 'primary/master' into ATH-allow-local-dev

dbf0fc2

kzantow mentioned this pull request Oct 23, 2017

testing is very hard #1503

Closed

kzantow merged commit c2c5134 into jenkinsci:master Oct 23, 2017

cliffmeyers mentioned this pull request Nov 3, 2017

Task/jenkins 36641 smart web element improvements #1529

Closed

7 tasks

[ATH IMPROVEMENT] Allow local Jenkins instance, simplified API #1484

[ATH IMPROVEMENT] Allow local Jenkins instance, simplified API #1484

Conversation

kzantow commented Oct 12, 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

michaelneale commented Oct 12, 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

cliffmeyers commented Oct 12, 2017

kzantow commented Oct 12, 2017

imeredith commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffmeyers left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelneale commented Oct 22, 2017

Choose a reason for hiding this comment

michaelneale commented Oct 23, 2017

michaelneale commented Oct 23, 2017 • edited Loading

michaelneale commented Oct 23, 2017 • edited Loading

sophistifunk commented Oct 23, 2017

kzantow commented Oct 23, 2017

sophistifunk commented Oct 23, 2017

michaelneale commented Oct 23, 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

cliffmeyers left a comment

Choose a reason for hiding this comment

kzantow commented Oct 23, 2017

kzantow commented Oct 23, 2017

michaelneale commented Oct 23, 2017 •

edited

Loading

michaelneale commented Oct 23, 2017 •

edited

Loading