-
Notifications
You must be signed in to change notification settings - Fork 2.8k
selenium test spark, pyspark and sparkSql #654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
52b74f1 to
d49344f
Compare
|
Ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, but how do you think, is it possible to improve this part (and alike below) a bit, to something more readable and easier to maintain?
May be extracting variables for Keys.chord(Keys.SHIFT, "9") and giving them a short but descriptive names like enter, or eol, etc could help
586ea6c to
5187a16
Compare
|
@prabhjyotsingh thanks for taking care of reivew in timely manner! It looks great, how do you think can it be further improved by |
b5a1644 to
76ad5b0
Compare
…selenium.Keys.* CI fix for pyspark is not supported in spark version 1.1.x
76ad5b0 to
ab03287
Compare
|
@bzz thank you for the quick review, did the relevant change. |
|
Very nice useage of Probably a knit-piking but may be also Looks great to me. I'm just curious, may be you know, why open parentesys is a key, but closing one seems to be ok inside the string literal? 👍 for more tests that help improving stability! |
|
@bzz yes thank you. Did it for Some javascript associated with ace editor is causing problem with open parenthesis, and other that are defined in HelperKeys, and hence have to use this (OPEN_PARENTHESIS). |
|
Ready for review. |
|
Great! |
|
Sure @felixcheung, this sounds better, have create a new PR #706 for addressing this. |
# Conflicts: # zeppelin-server/src/test/java/org/apache/zeppelin/integration/ZeppelinIT.java
| waitForParagraph(1, "FINISHED"); | ||
| } catch (TimeoutException e) { | ||
| waitForParagraph(1, "ERROR"); | ||
| collector.checkThat("Paragraph result resulted in error ::", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible in the check and LOG.error to specify this is from the sql spark test?
remove test for spark 1.1.1 more meaningful log message
|
Thanks @felixcheung for the review. Made appropriate changes. |
|
I assume this is not running the tests, but only skipping? |
|
Yes skipping since the env "TEST_SELENIUM" is not set. |
|
cool LGTM. merging if no more comment |
What is this PR for?
Test functionality of spark, pyspark, sparksql
What type of PR is it?
Improvement
Todos
Is there a relevant Jira issue?
ZEPPELIN-587
How should this be tested?
On macOS