Skip to content

Conversation

@prabhjyotsingh
Copy link
Contributor

@prabhjyotsingh prabhjyotsingh commented Jun 27, 2016

What is this PR for?

This is fix for fixing flaky CI failing test

testRemoveButton(org.apache.zeppelin.integration.ParagraphActionsIT)  Time elapsed: 9.641 sec  <<< FAILURE!
java.lang.AssertionError: After Remove : Number of paragraphs are
Expected: <2>
     but: was <1>
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
    at org.junit.Assert.assertThat(Assert.java:865)
    at org.junit.rules.ErrorCollector$1.call(ErrorCollector.java:65)
    at org.junit.rules.ErrorCollector.checkSucceeds(ErrorCollector.java:78)
    at org.junit.rules.ErrorCollector.checkThat(ErrorCollector.java:63)
    at org.apache.zeppelin.integration.ParagraphActionsIT.testRemoveButton(ParagraphActionsIT.java:161)

What type of PR is it?

[Bug Fix]

Todos

  • - Add delay after paragraph delete.

What is the Jira issue?

How should this be tested?

CI should be green

Questions:

  • Does the licenses files need update? n/a
  • Is there breaking changes for older versions? n/a
  • Does this needs documentation? n/a

@prabhjyotsingh prabhjyotsingh changed the title [ZEPPELIN-1065] add delay after deleting paragraph. [ZEPPELIN-1065] Flaky Test - ParagraphActionsIT.testRemoveButton Jun 27, 2016
sleep(1000, true);
driver.findElement(By.xpath("//div[@class='modal-dialog'][contains(.,'delete this paragraph')]" +
"//div[@class='modal-footer']//button[contains(.,'OK')]")).click();
sleep(1000, true);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to use this method vs ZeppelinITUtils.sleep(1000, false); below in the same method?

If they are the same, may be while we are here it can be refactored to use the same impl?

@bzz
Copy link
Member

bzz commented Jun 27, 2016

@prabhjyotsingh thank you for fixing this one!

What do you think, if would it make sense to reduce total wait time and get this test more stable with a wait polling implementation to locate such elements, instead of explicit wait everywhere?

I.e there are already one for text and paragraph here AbstractZeppelinIT.waitForText wich uses AbstractZeppelinIT.pollingWait underneath.

Would it make sense to add something like AbstractZeppelinIT.waitAndClick()?

 - use clickAndWait before reading any WebElement
@prabhjyotsingh
Copy link
Contributor Author

@bzz fair point;

  • have refactored all AbstractZeppelinIT's sleep calls into ZeppelinITUtils.
  • added another function clickAndWait, which will click and wait for 1s.

@prabhjyotsingh
Copy link
Contributor Author

CI fails for ZEPPELIN-1063


protected void clickAndWait(final By locator) {
driver.findElement(locator).click();
ZeppelinITUtils.sleep(1000, true);
Copy link
Member

Choose a reason for hiding this comment

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

Looks great!
Are there any reason not to use polling facilities of pollingWait ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, just waiting for any animation, show/hide or any (JavaScript) binding to the element that might be happening after it is visible.

@bzz
Copy link
Member

bzz commented Jun 27, 2016

Thank you for pointing out ZEPPELIN-1063 - have updated it's description and will fix it asap.

@prabhjyotsingh
Copy link
Contributor Author

I'll merge this, and make clickAndWait related changes in #1092.

@bzz
Copy link
Member

bzz commented Jun 27, 2016

Sounds good, and this PR looks great to me, +1 for merging asap

@asfgit asfgit closed this in c456967 Jun 27, 2016
@bzz
Copy link
Member

bzz commented Jun 27, 2016

@prabhjyotsingh on closing the JIRA issues, please do not forget to set correct fix version field - it simplifies Release Manager's job tremendously.

Curren one is 0.7.0 if changes were merged only to master. I have updated 1065

@prabhjyotsingh prabhjyotsingh deleted the ZEPPELIN-1065 branch February 25, 2018 03:44
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.

2 participants