Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SPARK-12034][SPARKR] Eliminate warnings in SparkR test cases. #10030

Closed
wants to merge 3 commits into from
Closed

[SPARK-12034][SPARKR] Eliminate warnings in SparkR test cases. #10030

wants to merge 3 commits into from

Conversation

sun-rui
Copy link
Contributor

@sun-rui sun-rui commented Nov 29, 2015

This PR:

  1. Suppress all known warnings.
  2. Cleanup test cases and fix some errors in test cases.
  3. Fix errors in HiveContext related test cases. These test cases are actually not run previously due to a bug of creating TestHiveContext.
  4. Support 'testthat' package version 0.11.0 which prefers that test cases be under 'tests/testthat'
  5. Make sure the default Hadoop file system is local when running test cases.
  6. Turn on warnings into errors.

@sun-rui
Copy link
Contributor Author

sun-rui commented Nov 29, 2015

This PR depends on SPARK-11781, will rebase it after the PR for SPARK-11781 get merged into master.

@SparkQA
Copy link

SparkQA commented Nov 29, 2015

Test build #46850 has finished for PR 10030 at commit 7e78d0f.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

felixcheung
felixcheung reviewed Nov 29, 2015
View reviewed changes
R/pkg/inst/tests/testthat/test_mllib.R
@@ -26,7 +26,7 @@ sc <- sparkR.init()
sqlContext <- sparkRSQL.init(sc)

test_that("glm and predict", {
training <- createDataFrame(sqlContext, iris)
training <- suppressWarnings(createDataFrame(sqlContext, iris))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a note in JIRA SPARK-11976 that once that is fixed we should remove suppressWarnings around DF from iris?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are a number of suppressWarnings in this PR. Instead of adding notes for them in this PR, I'd like to add a note in SPARK-11976 that cleaning these warnings is part of the job. Is it OK?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, that's what I mean, add a note in SPARK-11976 to remove these.
alternatively, we could open another JIRA to change all these tests to use a different data frame that doesn't have column names with .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added note in SPARK-11976

@sun-rui
Copy link
Contributor Author

sun-rui commented Nov 30, 2015

rebased to master

@SparkQA
Copy link

SparkQA commented Nov 30, 2015

Test build #46867 has finished for PR 10030 at commit d2d4edb.

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

@shivaram
Copy link
Contributor

Hmm the recommendation in testthat [1] seems to be to put the tests in R/pkg/tests/testthat instead of R/pkg/inst/tests -- Could we try to do that ? Or are there any problems with that ?

[1] https://github.com/hadley/testthat#integration-with-r-cmd-check

@sun-rui
Copy link
Contributor Author

sun-rui commented Nov 30, 2015

@shivaram, I tried , that does not work. It seems right to put test cases in R/pkg/inst/tests/testthat, and after installation, the cases cases are available at 'R/lib/SparkR/tests/testthat'. I think tests/testthat means the path after installation.

@sun-rui
Copy link
Contributor Author

sun-rui commented Dec 4, 2015

@shivaram, could we merge this?

@shivaram
Copy link
Contributor

shivaram commented Dec 4, 2015

@sun-rui Could you bring this update to date with master ?

@sun-rui
Copy link
Contributor Author

sun-rui commented Dec 4, 2015

rebased to master

@SparkQA
Copy link

SparkQA commented Dec 4, 2015

Test build #47187 has finished for PR 10030 at commit 9156e8c.

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

@shivaram
Copy link
Contributor

shivaram commented Dec 6, 2015

I'm sorry @sun-rui -- looks like some other merge conflicts now. Could you ping this thread once its up to date ? I will merge this PR asap after that.

@sun-rui
Copy link
Contributor Author

sun-rui commented Dec 7, 2015

@shivaram, rebased again. I can successfully rebase it to master on my dev machine, don't know any other merge conflicts. Could you merge it before merging any other PR?

@SparkQA
Copy link

SparkQA commented Dec 7, 2015

Test build #47257 has finished for PR 10030 at commit 37d8080.

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

@shivaram
Copy link
Contributor

shivaram commented Dec 7, 2015

LGTM. Thanks @sun-rui -- Merging this to master and 1.6

asfgit pushed a commit that referenced this pull request Dec 7, 2015
@shivaram
[SPARK-12034][SPARKR] Eliminate warnings in SparkR test cases.
3f230f7 
This PR:
1. Suppress all known warnings.
2. Cleanup test cases and fix some errors in test cases.
3. Fix errors in HiveContext related test cases. These test cases are actually not run previously due to a bug of creating TestHiveContext.
4. Support 'testthat' package version 0.11.0 which prefers that test cases be under 'tests/testthat'
5. Make sure the default Hadoop file system is local when running test cases.
6. Turn on warnings into errors.

Author: Sun Rui <[email protected]>

Closes #10030 from sun-rui/SPARK-12034.

(cherry picked from commit 39d677c)
Signed-off-by: Shivaram Venkataraman <[email protected]>
@asfgit asfgit closed this in 39d677c Dec 7, 2015
This was referenced Dec 7, 2015
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sun-rui @SparkQA @shivaram @felixcheung