Skip to content

Commit

Permalink
[SPARK-29777][FOLLOW-UP][SPARKR] Remove no longer valid test for recu…
Browse files Browse the repository at this point in the history
…rsive calls

### What changes were proposed in this pull request?

Disabling test for cleaning closure of recursive function.

### Why are the changes needed?

As of 9514b82 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 (#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 <mszymkiewicz@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
  • Loading branch information
zero323 authored and HyukjinKwon committed Jan 26, 2020
1 parent 1b3ddcf commit 81ea5a4
Showing 1 changed file with 8 additions and 2 deletions.
10 changes: 8 additions & 2 deletions R/pkg/tests/fulltests/test_utils.R
Expand Up @@ -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)
Expand All @@ -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)
Expand Down

0 comments on commit 81ea5a4

Please sign in to comment.