testSparkInterpreterDependencyLoading does not check for endToEndTestEnabled #709

prabhjyotsingh · 2016-02-10T05:28:07Z

What is this PR for?

This is a fix for selenium test case testSparkInterpreterDependencyLoading that does not check for endToEndTestEnabled causing build to fail on local. (mvn $TEST_FLAG $PROFILE -B)

Used to get this error in file ./zeppelin-server/target/failsafe-reports/org.apache.zeppelin.ZeppelinIT.txt

-------------------------------------------------------------------------------
Test set: org.apache.zeppelin.ZeppelinIT
-------------------------------------------------------------------------------
Tests run: 2, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.008 sec <<< FAILURE! - in org.apache.zeppelin.ZeppelinIT
testSparkInterpreterDependencyLoading(org.apache.zeppelin.ZeppelinIT)  Time elapsed: 0.008 sec      <<< ERROR!
java.lang.NullPointerException: null
    at org.apache.zeppelin.ZeppelinIT.testSparkInterpreterDependencyLoading(ZeppelinIT.java:210)

What type of PR is it?

Hot Fix

Todos

- Fix for testSparkInterpreterDependencyLoading
- delete created notebook for cleanup.

Is there a relevant Jira issue?

N/A

How should this be tested?

mvn $TEST_FLAG $PROFILE -B should not fail on local when env variable "CI" is not set.

felixcheung · 2016-02-10T07:47:25Z

zeppelin-server/src/test/java/org/apache/zeppelin/ZeppelinIT.java

+      testDepRemoveBtn.click();
+      driver.findElement(By.xpath("//button[contains(.,'Save')]")).submit();
+      driver.switchTo().alert().accept();
+    } catch (ElementNotVisibleException e) {


would there be any other error? like xpath returning no match (say the element has been removed)

Sure, what you say make sense, let me change all other catch from ElementNotVisibleException to just Exception, so, it catches every thing in #706

felixcheung · 2016-02-10T07:49:24Z

I think it's fine keeping this in #706 - either way is ok with me.
Much easier to see diff this way: https://github.com/apache/incubator-zeppelin/pull/709/files?w=1

prabhjyotsingh · 2016-02-10T08:20:50Z

Yes, agreed, this is much cleaner.

…Enabled

…n" and log it.

prabhjyotsingh · 2016-02-11T03:59:00Z

@felixcheung have replaced all "ElementNotVisibleException" to "Exception", and then log the same.

felixcheung · 2016-02-11T04:21:47Z

looks good.

Leemoonsoo · 2016-02-11T05:36:04Z

zeppelin-server/src/test/java/org/apache/zeppelin/ZeppelinIT.java

+    } catch (Exception e) {
+      LOG.error("Exception in ZeppelinIT while testSparkInterpreterDependencyLoading ", e);
+      File scrFile = ((TakesScreenshot) driver).getScreenshotAs(OutputType.FILE);
+


Shouldn't we throw e after taking screenshot, if getting exception is considered as a test failure?

Sure did the relevant change.

You might want to just throw e - exception stack should already have the test name?

Thanks a lot @felixcheung, made the required change.

…umFix

prabhjyotsingh · 2016-02-12T05:38:46Z

@felixcheung @Leemoonsoo Thanks for the review. I have addressed the comment.

felixcheung · 2016-02-12T08:10:55Z

zeppelin-server/src/test/java/org/apache/zeppelin/ZeppelinIT.java

      LOG.error("Exception in ZeppelinIT while testAngularDisplay ", e);
      File scrFile = ((TakesScreenshot)driver).getScreenshotAs(OutputType.FILE);
-
+      throw new Exception(e);


can we instead say throw e; here? e is already an Exception
http://stackoverflow.com/questions/1097527/rethrowing-exceptions-in-java-without-losing-the-stack-trace

got it!!! missed it. Thank you. :)

felixcheung · 2016-02-12T21:46:29Z

looks good, thanks.
merging if no more comments.

Leemoonsoo · 2016-02-13T07:25:11Z

LGTM

@bzz

### What is this PR for? Test functionality of spark, pyspark, sparksql ### What type of PR is it? Improvement ### Todos * [x] - Selenium for spark * [x] - Selenium for pyspark * [x] - Selenium for sparksql * [x] - refactor with #619 ### Is there a relevant Jira issue? ZEPPELIN-587 ### How should this be tested? On macOS PATH=~/Applications/Firefox.app/Contents/MacOS/:$PATH CI="" \ mvn -Dtest=org.apache.zeppelin.integration.TestSparkParagraph -Denforcer.skip=true \ test -pl zeppelin-server Author: Prabhjyot Singh <prabhjyotsingh@gmail.com> Closes #654 from prabhjyotsingh/ZEPPELIN-587 and squashes the following commits: 8f24695 [Prabhjyot Singh] use handleException in all other test cases remove test for spark 1.1.1 more meaningful log message 28dfc55 [Prabhjyot Singh] thorwong exception similar to #709 21bcc45 [Prabhjyot Singh] Merge remote-tracking branch 'origin/master' into ZEPPELIN-587 b05b81b [Prabhjyot Singh] have SHIFT_ENTER enum 9a206f4 [Prabhjyot Singh] add missing endToEndTestEnabled check for testSparkInterpreterDependencyLoading ab03287 [Prabhjyot Singh] have static import of AbstractZeppelinIT.HelperKeys.*, and rg.openqa.selenium.Keys.* 5187a16 [Prabhjyot Singh] check if spark version is less than 1.3 then don't append ".toDF()" 6927f7e [Prabhjyot Singh] CI FIX 9236e3c [Prabhjyot Singh] implemeting @bzz review comments 2c28758 [Prabhjyot Singh] missed refactor with #619 d49344f [Prabhjyot Singh] selenium test spark, pyspark and sparkSql

prabhjyotsingh mentioned this pull request Feb 10, 2016

Create seperate matrix for selenium test case #706

Closed

prabhjyotsingh force-pushed the ZEPPELIN-630-SeleniumFix branch from bb8ebb5 to 73e6334 Compare February 10, 2016 07:22

felixcheung reviewed Feb 10, 2016
View reviewed changes

testSparkInterpreterDependencyLoading does not check for endToEndTest…

2642975

…Enabled

prabhjyotsingh force-pushed the ZEPPELIN-630-SeleniumFix branch from 73e6334 to 2642975 Compare February 10, 2016 17:30

instead of "ElementNotVisibleException" it should catch all "Exceptio…

271445a

…n" and log it.

Leemoonsoo reviewed Feb 11, 2016
View reviewed changes

prabhjyotsingh added 2 commits February 11, 2016 11:17

throw e on exception, marking test as a failure.

98e8d0b

Merge remote-tracking branch 'origin/master' into ZEPPELIN-630-Seleni…

277521e

…umFix

prabhjyotsingh force-pushed the ZEPPELIN-630-SeleniumFix branch from 35ae3c9 to c60858d Compare February 12, 2016 03:51

"throw e"

2d9aa98

prabhjyotsingh force-pushed the ZEPPELIN-630-SeleniumFix branch from c60858d to 2d9aa98 Compare February 12, 2016 05:05

felixcheung reviewed Feb 12, 2016
View reviewed changes

throw e;

e31a3d0

asfgit closed this in 72b8fde Feb 14, 2016

prabhjyotsingh added a commit to prabhjyotsingh/zeppelin that referenced this pull request Feb 16, 2016

thorwong exception similar to apache#709

28dfc55

prabhjyotsingh mentioned this pull request Feb 16, 2016

selenium test spark, pyspark and sparkSql #654

Closed

4 tasks

prabhjyotsingh deleted the ZEPPELIN-630-SeleniumFix branch February 24, 2016 05:25

testSparkInterpreterDependencyLoading does not check for endToEndTestEnabled #709

testSparkInterpreterDependencyLoading does not check for endToEndTestEnabled #709

Uh oh!

Conversation

prabhjyotsingh commented Feb 10, 2016

What is this PR for?

What type of PR is it?

Todos

Is there a relevant Jira issue?

How should this be tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Feb 10, 2016

Uh oh!

prabhjyotsingh commented Feb 10, 2016

Uh oh!

prabhjyotsingh commented Feb 11, 2016

Uh oh!

felixcheung commented Feb 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prabhjyotsingh commented Feb 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Feb 12, 2016

Uh oh!

Leemoonsoo commented Feb 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants