Skip to content

Conversation

@zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Sep 27, 2016

What is this PR for?

This PR fix several issues of LivyInterpeter test.

  • Livy interpreter's test code is not in the right place, so it never runs.
  • LivyHelperTest would fail.
  • No Integration test for LivyInterpreter so any following change is not easy to be tested.

This PR would fix the above issues. Regarding the integration test, some of livy's artifact is not available in repository, so I have to copy them to livy/local-maven-repo as local repository. And LivyInterperter's integration test require spark and livy to be installed. For now you have to download spark and livy manually. Please use spark 1.5.x and livy 0.2 which is currently supported. Download livy 0.2.0 from here https://github.com/cloudera/livy/releases

And use the following command to execute the LivyIntegrationTest.

export SPARK_HOME=<path_to_spark>
export LIVY_HOME=<path_to_livy>
mvn clean package -pl 'livy' -Dtest=LivyIntegrationTest

If you hit any issues, you can check logs under livy/target/tmp.

What type of PR is it?

[Bug Fix | Improvement]

Todos

  • - Task

What is the Jira issue?

Screenshots (if appropriate)

image

Questions:

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

@zjffdu zjffdu closed this Sep 27, 2016
@zjffdu zjffdu reopened this Sep 27, 2016
@zjffdu
Copy link
Contributor Author

zjffdu commented Sep 27, 2016

@Leemoonsoo @prabhjyotsingh @felixcheung Please help review, the failed test is irrelevant, I have created ZEPPELIN-1498 for it.

@felixcheung
Copy link
Member

is there a way we could avoid checking in jar? that would make release a bit trickier?

@zjffdu
Copy link
Contributor Author

zjffdu commented Sep 28, 2016

I am afraid this is the only workaround for now. Because these jars are not available in maven repository.

@zjffdu
Copy link
Contributor Author

zjffdu commented Sep 28, 2016

I have asked the livy community to publish these artifacts, suppose I can delete these livy artifacts in zeppelin after livy 0.3 is released.

@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 7, 2016

reping @Leemoonsoo @prabhjyotsingh @bzz @jongyoul @felixcheung please help review.

@zjffdu zjffdu closed this Oct 7, 2016
@zjffdu zjffdu reopened this Oct 7, 2016
@prabhjyotsingh
Copy link
Contributor

prabhjyotsingh commented Oct 10, 2016

I too share same thought as @felixcheung, if we can avoid jar(s), can we use org.mockito.Mock instead ?

@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 10, 2016

@prabhjyotsingh These jars are not available in public maven repository. Do you have any better ideas ?

@prabhjyotsingh
Copy link
Contributor

@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 10, 2016

@prabhjyotsingh I dont' get it. This is integration test where I will launch a real livy server, how can I use mockito to simulate that ?

@prabhjyotsingh
Copy link
Contributor

What I mean to say was, lets mock these till the time we get livy's jar from public repo, how do you think about that ?

@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 10, 2016

mockit is for unit test, not for system test. It is just like AbstractTestRestApi where we will launch real spark cluster and ZeppelinServer. Using mockit can not cover lots of cases in real enviroment (e.g. how can I test livy handle comment correctly (considering we are doing some preprocessing), how can I test SparkContext is shared correctly between %livy.spark and %livy.sql) . I know committing jars are not good practise, but these jars won't be available in public repository in short time.

@prabhjyotsingh
Copy link
Contributor

Yes, that part I agree, it cannot be tested without jar, org.mockito.Mock will always mock.

@Leemoonsoo @jongyoul @felixcheung any thought about checking in jar(s) ?

@felixcheung
Copy link
Member

As I have stated, I think these tests are good to have but worry about the added complicity for a source release having these jars in the "source", especially since these jars are not from an ASF project.

@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 11, 2016

Livy interpreter is not a trivial interpreter although it use rest api. The current unit test is not sufficient to guarantee the quality of livy interpreter. I have found some issues there, but before doing improvement and fix, I would like to commit this integration test, otherwise I have to manually test it after each fix and refactor. I know committing jars is not a good practise, but this is the only workaround I can think of.

@zjffdu zjffdu closed this Oct 17, 2016
@zjffdu zjffdu reopened this Oct 17, 2016
@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 20, 2016

@felixcheung @prabhjyotsingh I have published livy jars to OSSRH, and now I removed the livy jars. Please help review.

@prabhjyotsingh
Copy link
Contributor

prabhjyotsingh commented Oct 20, 2016

I just tried it on my local; so basically it skips all the test cases (unless yes i provide LIVY_HOME, and SPARK_HOME).

WARN [2016-10-20 18:17:05,017] ({main} LivyIntegrationTest.java[checkPreCondition]:66) - livy integration is skipped because LIVY_HOME is not set
 WARN [2016-10-20 18:17:05,018] ({main} LivyIntegrationTest.java[checkPreCondition]:66) - livy integration is skipped because LIVY_HOME is not set
 WARN [2016-10-20 18:17:05,019] ({main} LivyIntegrationTest.java[checkPreCondition]:66) - livy integration is skipped because LIVY_HOME is not set
 WARN [2016-10-20 18:17:05,008] ({main} LivyIntegrationTest.java[checkPreCondition]:66) - livy integration is skipped because LIVY_HOME is not set

Any plan for this to merge with travis ?

@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 20, 2016

As mentioned in the PR description, for now you need to export SPARK_HOME and LIVY_HOME. I plan to merge with travis in the following PR.

@prabhjyotsingh
Copy link
Contributor

Otherwise yes, LGTM!

@felixcheung
Copy link
Member

One question - I'm not sure if dependencies for test only code should be listed in LICENSE.
Otherwise LGTM.

@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 24, 2016

@zjffdu
Copy link
Contributor Author

zjffdu commented Oct 24, 2016

ping @felixcheung @prabhjyotsingh

@felixcheung
Copy link
Member

felixcheung commented Oct 25, 2016 via email

@asfgit asfgit closed this in 0af86e3 Oct 26, 2016
darionyaphet pushed a commit to darionyaphet/zeppelin that referenced this pull request Oct 27, 2016
### What is this PR for?
This PR fix several issues of LivyInterpeter test.
* Livy interpreter's test code is not in the right place, so it never runs.
* LivyHelperTest would fail.
* No Integration test for LivyInterpreter so any following change is not easy to be tested.

This PR would fix the above issues. Regarding the integration test, some of livy's artifact is not available in repository, so I have to copy them to livy/local-maven-repo as local repository.  And LivyInterperter's integration test require spark and livy to be installed. For now you have to download spark and livy manually. Please use spark 1.5.x and livy 0.2 which is currently supported. Download livy 0.2.0 from here [https://github.com/cloudera/livy/releases](https://github.com/cloudera/livy/releases)

And use the following command to execute the LivyIntegrationTest.
```
export SPARK_HOME=<path_to_spark>
export LIVY_HOME=<path_to_livy>
mvn clean package -pl 'livy' -Dtest=LivyIntegrationTest
```
If you hit any issues, you can check logs under `livy/target/tmp`.

### What type of PR is it?
[Bug Fix | Improvement]

### Todos
* [ ] - Task

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-1477

### Screenshots (if appropriate)
![image](https://cloud.githubusercontent.com/assets/164491/18861677/b1389622-84b9-11e6-8b0a-424457ded975.png)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jeff Zhang <[email protected]>

Closes apache#1462 from zjffdu/ZEPPELIN-1477 and squashes the following commits:

75914b9 [Jeff Zhang] remove livy local jars
fee61f9 [Jeff Zhang] add more test
1b9fbbc [Jeff Zhang] add missing dependencies
e8ceff5 [Jeff Zhang] add missing livy jars
8632466 [Jeff Zhang] fix rat check
f560a92 [Jeff Zhang] ZEPPELIN-1477. Add Integration Test for LivyInterpreter
pedrozatta pushed a commit to pedrozatta/zeppelin that referenced this pull request Oct 27, 2016
### What is this PR for?
This PR fix several issues of LivyInterpeter test.
* Livy interpreter's test code is not in the right place, so it never runs.
* LivyHelperTest would fail.
* No Integration test for LivyInterpreter so any following change is not easy to be tested.

This PR would fix the above issues. Regarding the integration test, some of livy's artifact is not available in repository, so I have to copy them to livy/local-maven-repo as local repository.  And LivyInterperter's integration test require spark and livy to be installed. For now you have to download spark and livy manually. Please use spark 1.5.x and livy 0.2 which is currently supported. Download livy 0.2.0 from here [https://github.com/cloudera/livy/releases](https://github.com/cloudera/livy/releases)

And use the following command to execute the LivyIntegrationTest.
```
export SPARK_HOME=<path_to_spark>
export LIVY_HOME=<path_to_livy>
mvn clean package -pl 'livy' -Dtest=LivyIntegrationTest
```
If you hit any issues, you can check logs under `livy/target/tmp`.

### What type of PR is it?
[Bug Fix | Improvement]

### Todos
* [ ] - Task

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-1477

### Screenshots (if appropriate)
![image](https://cloud.githubusercontent.com/assets/164491/18861677/b1389622-84b9-11e6-8b0a-424457ded975.png)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jeff Zhang <[email protected]>

Closes apache#1462 from zjffdu/ZEPPELIN-1477 and squashes the following commits:

75914b9 [Jeff Zhang] remove livy local jars
fee61f9 [Jeff Zhang] add more test
1b9fbbc [Jeff Zhang] add missing dependencies
e8ceff5 [Jeff Zhang] add missing livy jars
8632466 [Jeff Zhang] fix rat check
f560a92 [Jeff Zhang] ZEPPELIN-1477. Add Integration Test for LivyInterpreter
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.

3 participants