[SPARK-20197][SPARKR] CRAN check fail with package installation

[felixcheung]

What changes were proposed in this pull request?

Test failed because SPARK_HOME is not set before Spark is installed.

[SparkQA]

Test build #75469 has finished for PR 17516 at commit c89380e.

• This patch fails R style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[felixcheung]

unlike in Jenkins, R CMD check looks to be running each test R file with the current directory set to its directory (eg. spark/R/pkg/inst/tests/testthat/ for test_sparkSQL.R)

when running as R CMD check, SPARK_HOME = ~/.cache/spark/spark* whereas spark-warehouse and metastore_db are actually created under R/inst/tests/testthat (where the actual test .R files are)

unless there is a good way to run all tests the exact same way as R CMD check, we will need to revisit what we are checking here.

[shivaram]

Looking at this now

[shivaram]

So does the current patch pass R CMD check when run on the master branch ?

[SparkQA]

Test build #75589 has finished for PR 17516 at commit a3e8b35.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[shivaram]

Don't we also need the skip if cran statement ?

[felixcheung]

The test actually passes - just not looking at the right place. Skip if cran is not applicable in master since we are running tests as cran - if I add skip if cran it will just disable this test completely. This is why I opt to originally to update the test instead instead of skip if cran in master. But since we might be cutting a RC any time I want to fix the blocker first and get back to fix the test (which doesn't fail) when I get the chance. I will merge this today unless you have a concern?

[shivaram]

The test passes even when we run the R CMD check --as-cran from a different directory ? I thought the fix in branch-2.1 was to get around that (my understanding could be wrong)

[felixcheung]

There are two parts to the branch-2.1 fix

First, the reason why the test failed was because SPARK_HOME was not set before calling spark.install() when running as a package. This would not be a problem when in Jenkins, but only when running with R CMD check SparkR*.tgz. The fix was to move spark.install to earlier.

Second, even after the change, while testing it, I found that R CMD check was getting spark-warehouse etc in the testthat directory, NOT in SPARK_HOME - therefore that test would be essentially a no-op or always passes anyway. I made the call to disable it (with skip_if_cran), but that had the unintended effect of also turning off that test in Jenkins, as we are testing with --as-cran

And so the attempt here in this PR to fix this for real in master. Since we are rolling our RC anytime, I don't want to delay the first fix (install.spark) only to sort out the 2nd part, which could come a bit later.

If you feel that's safer, we could also add skip_if_cran to this test in master - just know that it will also turn off this test in Jenkins. Since with R CMD check the spark-warehouse and metastore_db are not written to SPARK_HOME, but to testthat, this test will pass during the package test with R CMD check - so long as we merge this PR to move install.spark first

[shivaram]

Got it. LGTM. Thanks for explanation. I'm fine with merging this to master !

[felixcheung]

thanks, I find it rather odd but probably by design that the current directory is different when running R CMD check .tgz. will need to look at the more

[felixcheung]

merged to master