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

Restore test_dir(..., wrap = FALSE) #1183

Merged
merged 9 commits into from
Oct 5, 2020
Merged

Restore test_dir(..., wrap = FALSE) #1183

merged 9 commits into from
Oct 5, 2020

Conversation

brodieG
Copy link
Contributor

@brodieG brodieG commented Sep 27, 2020

In some unusual cases it is useful to be able to preserve the
original testthat behavior of not wrapping expressions outside
of test_that blocks.

In commit 37cc0d0
control of the wrap functionality was removed from test_dir,
but after discussion in #498 it was restored.

More recently, b13ecb3
removed the wrap functionality again from test_dir. This patch
seeks to restore the functionality for serial tests only as restoring it
for parallel tests is more complicated and of dubious value since
a) the parameter is now deprecated, and b) parallel tests did not
exist prior to deprecation.

Bonus typo fixes.

@brodieG
Copy link
Contributor Author

brodieG commented Sep 27, 2020

Hmm, I'll need to look into why the CI tests are failing despite having
worked locally for me.

In the meantime, @hadley any feedback on whether the concept of
this patch is okay for your or not would be useful for me to know.

hadley
hadley reviewed Sep 28, 2020
View reviewed changes
R/test-files.R Outdated
@@ -150,10 +163,18 @@ test_files <- function(test_dir,
env = NULL,
stop_on_failure = FALSE,
stop_on_warning = FALSE,
wrap = lifecycle::deprecated(),
Copy link
Member

Choose a reason for hiding this comment

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

Just make this TRUE and then warn if wrap is FALSE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

hadley
hadley reviewed Sep 28, 2020
View reviewed changes
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Overall, it's fine, but I made a couple of suggestions for simplifying it, since you're the only person who needs it.

R/test-files.R Outdated
@@ -98,6 +94,22 @@ test_dir <- function(path,
reporter <- find_reporter(reporter)
parallel <- want_parallel && reporter$capabilities$parallel_support

if (parallel) {
if (!is_missing(wrap)) {
Copy link
Member

Choose a reason for hiding this comment

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

Given that you're the only person using this argument, I think you can just keep the warning the same for both serial and parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I did this. If for whatever reason someone ends up using wrap=FALSE in parallel mode they might be confused, but given they will get a deprecation warning hopefully they'll figure it out, and more likely such a person won't exist anyway.

@brodieG
simplify patch per HW comments
e7733d5 
Note we use the 'test_dir' deprecation warning in
'test_files' as that line may be triggered from either
'test_file' or 'test_dir'.  In the docs 'test_file' is
described as 'a variant of test_dir()...' so it seems
ok for 'test_file' to trigger a deprecation warning
with 'test_dir' in it.
@brodieG brodieG mentioned this pull request Sep 30, 2020
Merged
@brodieG
Copy link
Contributor Author

brodieG commented Sep 30, 2020 •
edited
Loading

In re failing tests, I looked at this for a bit, and it's going wrong with errors of this form (among others):

── Failure (Line 10): error in parallel setup code ─────────────────────────────
`err` inherits from `rlib_error/error/condition` not `testthat_process_error`.

── Failure (Line 11): error in parallel setup code ─────────────────────────────
err$message does not match "Error in setup".
Actual value: "R session finished"

Or:

── Failure (Line 11): error in parallel setup code ─────────────────────────────
err$message does not match "Error in setup".
Actual value: "R session not ready yet"

I could not reproduce locally on OS X R4.0.2, but did reproduce it on Ubuntu 18.04 with r-devel. The error seems related to the callr sessions. I'm pretty sure this is not my doing as I reproduced the error with the HEAD version of testthat, and the errors appear in #1184 as well.

@hadley hadley merged commit 1027138 into r-lib:master Oct 5, 2020
@hadley
Copy link
Member

hadley commented Oct 5, 2020

Thanks!

@brodieG brodieG mentioned this pull request Oct 6, 2020
Closed
@brodieG
Copy link
Contributor Author

brodieG commented Oct 2, 2021

Update here: I no longer rely on the ability to turn wrap off. I know this is something you'd been hoping to remove, so just letting you know that my packages will no longer be affected if you do deprecate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hadley hadley hadley left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brodieG @hadley