[spark] branch branch-2.4 updated: [SPARK-29777][FOLLOW-UP][SPARKR] Remove no longer valid test for recursive calls

[gu...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

gurwls223 pushed a commit to branch branch-2.4
in repository https://gitbox.apache.org/repos/asf/spark.git


The following commit(s) were added to refs/heads/branch-2.4 by this push:
     new 81ea5a4  [SPARK-29777][FOLLOW-UP][SPARKR] Remove no longer valid test for recursive calls
81ea5a4 is described below

commit 81ea5a4cc1617de0dbbc61811847320724b3644f
Author: zero323 <ms...@gmail.com>
AuthorDate: Sat Jan 25 21:16:22 2020 -0800

    [SPARK-29777][FOLLOW-UP][SPARKR] Remove no longer valid test for recursive calls
    
    ### What changes were proposed in this pull request?
    
    Disabling test for cleaning closure of recursive function.
    
    ### Why are the changes needed?
    
    As of https://github.com/apache/spark/commit/9514b822a70d77a6298ece48e6c053200360302c this test is no longer valid, and recursive calls, even simple ones:
    
    ```lead
      f <- function(x) {
        if(x > 0) {
          f(x - 1)
        } else {
          x
        }
      }
    ```
    
    lead to
    
    ```
    Error: node stack overflow
    ```
    
    This is issue is silenced when tested with `testthat` 1.x (reason unknown), but cause failures when using `testthat` 2.x (issue can be reproduced outside test context).
    
    Problem is known and tracked by [SPARK-30629](https://issues.apache.org/jira/browse/SPARK-30629)
    
    Therefore, keeping this test active doesn't make sense, as it will lead to continuous test failures, when `testthat` is updated (https://github.com/apache/spark/pull/27359 / SPARK-23435).
    
    ### Does this PR introduce any user-facing change?
    
    No.
    
    ### How was this patch tested?
    
    Existing tests.
    
    CC falaki
    
    Closes #27363 from zero323/SPARK-29777-FOLLOWUP.
    
    Authored-by: zero323 <ms...@gmail.com>
    Signed-off-by: Dongjoon Hyun <dh...@apple.com>
---
 R/pkg/tests/fulltests/test_utils.R | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/R/pkg/tests/fulltests/test_utils.R b/R/pkg/tests/fulltests/test_utils.R
index b2b6f34..ff52ced 100644
--- a/R/pkg/tests/fulltests/test_utils.R
+++ b/R/pkg/tests/fulltests/test_utils.R
@@ -89,7 +89,10 @@ test_that("cleanClosure on R functions", {
     lapply(x, g) + 1  # Test for capturing function call "g"'s closure as a argument of lapply.
     l$field[1, 1] <- 3  # Test for access operators `$`.
     res <- defUse + l$field[1, ]  # Test for def-use chain of "defUse", and "" symbol.
-    f(res)  # Test for recursive calls.
+    # Enable once SPARK-30629 is fixed
+    # nolint start
+    # f(res)  # Test for recursive calls.
+    # nolint end
   }
   newF <- cleanClosure(f)
   env <- environment(newF)
@@ -101,7 +104,10 @@ test_that("cleanClosure on R functions", {
   # nolint end
   expect_true("g" %in% ls(env))
   expect_true("l" %in% ls(env))
-  expect_true("f" %in% ls(env))
+  # Enable once SPARK-30629 is fixed
+  # nolint start
+  # expect_true("f" %in% ls(env))
+  # nolint end
   expect_equal(get("l", envir = env, inherits = FALSE), l)
   # "y" should be in the environment of g.
   newG <- get("g", envir = env, inherits = FALSE)


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@spark.apache.org
For additional commands, e-mail: commits-help@spark.apache.org