-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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
[SPARK-9284] [tests] Allow all tests to run without an assembly. #7629
Conversation
Doing this may cause weird errors when tests are run on maven, depending on the flags used. Instead, expose the needed functionality through methods that do not expose shaded classes.
This change aims at speeding up the dev cycle a little bit, by making sure that all tests behave the same w.r.t. where the code to be tested is loaded from. Namely, that means that tests don't rely on the assembly anymore, rather loading all needed classes from the build directories. The main change is to make sure all build directories (classes and test-classes) are added to the classpath of child processes when running tests. YarnClusterSuite required some custom code since the executors are run differently (i.e. not through the launcher library, like standalone and Mesos do). I also found a couple of tests that could leak a SparkContext on failure, and added code to handle those. With this patch, it's possible to run the following command from a clean source directory and have all tests pass: mvn -Pyarn -Phadoop-2.4 -Phive-thriftserver install
Note I included a separate patch in this PR, since I was hitting that issue every time with these changes. I'll rebase once that patch is committed. |
Test build #38281 has finished for PR 7629 at commit
|
Weird that scalastyle passed locally. |
Test build #38387 has finished for PR 7629 at commit
|
Test build #38393 has finished for PR 7629 at commit
|
Test build #38401 has finished for PR 7629 at commit
|
Ok, this is down to that kinesis test that's failing for everybody. |
Jenkins, retest this please. |
Test build #119 has finished for PR 7629 at commit
|
Jenkins retest this please. |
Test build #138 has finished for PR 7629 at commit
|
Jenkins, retest this please. |
Test build #140 has finished for PR 7629 at commit
|
Test build #38774 has finished for PR 7629 at commit
|
retest this please |
Test build #182 has finished for PR 7629 at commit
|
Test build #39251 has finished for PR 7629 at commit
|
retest this please |
Test build #39280 has finished for PR 7629 at commit
|
Test build #183 has finished for PR 7629 at commit
|
Test build #39303 has finished for PR 7629 at commit
|
retest this please |
Test build #188 has finished for PR 7629 at commit
|
Test build #41471 timed out for PR 7629 at commit |
Seems like all tests passed, no idea why jenkins thinks they timed out. AFAICT, this is good to do. @pwendell ? |
Yeah sounds good - might be good to let it run one more time just to be On Mon, Aug 24, 2015 at 4:40 PM, Marcelo Vanzin notifications@github.com
|
Jenkins, test this please. |
Test build #41492 timed out for PR 7629 at commit |
175m is starting to look really low. the scala/java unit tests took 143m to run. anyway, retest this please. |
Does this PR increase test time in some way? Just wondering why this would On Tue, Aug 25, 2015 at 10:43 AM, Marcelo Vanzin notifications@github.com
|
No, it shouldn't affect the test runtime at all. And I get timeouts in lots of PRs, not just this one. |
Test build #41539 has finished for PR 7629 at commit
|
Jenkins, retest this please. |
Test build #41559 has finished for PR 7629 at commit
|
Test build #41574 has finished for PR 7629 at commit
|
I've seen the failing test fail in other PRs (and it has passed here before)... let's try again just in case. retest this please |
Test build #41584 timed out for PR 7629 at commit |
Marcelo you will need to change the timeout in the code itself for it to
|
Yes but I don't want to do that as part of this change, since they're unrelated things. All tests have been passing, the timeouts are unrelated to the PR. |
Okay then - I would do that separately, get this PR passing, then merge it. On Wed, Aug 26, 2015 at 9:15 AM, Marcelo Vanzin notifications@github.com
|
Often, but not deterministically every time. It seems to time out as often as any other PR that needs to run all tests (I've already cc'ed you on at least another one that times out just as often). |
I still don't understand - why not fix the issue in a separate PR, get this On Wed, Aug 26, 2015 at 9:46 AM, Marcelo Vanzin notifications@github.com
|
I'm working on fixing the root cause of the timeouts (running unnecessary tests). If you think it would be beneficial to just bump the timeout right now, please just send a PR for that; I'm pretty confident that this PR does not make the timeout issue any worse. |
Jenkins, retest this please. |
K - I just sent a hotfix to up the timeout. On Wed, Aug 26, 2015 at 9:51 AM, Marcelo Vanzin notifications@github.com
|
Test build #41647 has finished for PR 7629 at commit
|
Yay! |
Great - looks good!
|
Ok I'm merging this. |
This change aims at speeding up the dev cycle a little bit, by making
sure that all tests behave the same w.r.t. where the code to be tested
is loaded from. Namely, that means that tests don't rely on the assembly
anymore, rather loading all needed classes from the build directories.
The main change is to make sure all build directories (classes and test-classes)
are added to the classpath of child processes when running tests.
YarnClusterSuite required some custom code since the executors are run
differently (i.e. not through the launcher library, like standalone and
Mesos do).
I also found a couple of tests that could leak a SparkContext on failure,
and added code to handle those.
With this patch, it's possible to run the following command from a clean
source directory and have all tests pass:
mvn -Pyarn -Phadoop-2.4 -Phive-thriftserver install