[SPARK-29777][FOLLOW-UP][SPARKR] Remove no longer valid test for recursive calls #27363
Conversation
Signed-off-by: zero323 <mszymkiewicz@gmail.com>
| expect_true("f" %in% ls(env)) | ||
| # Enable once SPARK-30629 is fixed | ||
| # nolint start | ||
| # expect_true("f" %in% ls(env)) |
There was a problem hiding this comment.
I remember codes in comments break one of the R linter rule. Might be good to double check.
There was a problem hiding this comment.
They do, that's why it is wrapped with nolint. If I didn't mess up anything, this is cherry picked from previous version of SPARK-23435 proposal, and already passed all tests, but let's see how it goes.
|
Thanks @zero323 |
|
Test build #117409 has finished for PR 27363 at commit
|
There was a problem hiding this comment.
Thanks. Since SPARK-29777 is merged to only master, this is only for master. Is it correct?
|
Or, do we need to backport SPARK-29777? |
It is not my call, but I'd vote against backporting. Despite its value it introduces a breaking change, and as far as I understand, at this point we don't have good fix lined up. |
|
Thanks for the confirmation. Since this is not a correctness or dataloss issue, it seems to be okay for now. |
|
Thanks everyone! |
|
@dongjoon-hyun, I will backport this to branch-2.4 because if we upgrade R version, that will likely face this issue. |
…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>
|
Oh, wait SPARK-29777 is in 3.0 only ... Okay. I will revert it back for now. We can handle it later when we actually upgrade in a separate JIRA. |
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 to
This is issue is silenced when tested with
testthat1.x (reason unknown), but cause failures when usingtestthat2.x (issue can be reproduced outside test context).Problem is known and tracked by SPARK-30629
Therefore, keeping this test active doesn't make sense, as it will lead to continuous test failures, when
testthatis updated (#27359 / SPARK-23435).Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing tests.
CC @falaki