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.

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

Don't write to the test directory during testing #548

Merged
merged 9 commits into from May 9, 2020

Conversation

michaelquinn32
Copy link
Contributor

Trees and reference items are copied into a test directory. In certain testing environments, the test directory is not writeable. The only safe location to modify files is within tempdir()

Trees and reference items are copied into a test directory. In certain testing environments, the test directory is not writeable. The only safe location to modify files is within tempdir()
@codecov-io
Copy link

codecov-io commented Sep 24, 2019

Codecov Report

Merging #548 into master will decrease coverage by 0.24%.
The diff coverage is 46.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #548      +/-   ##
==========================================
- Coverage   90.64%   90.40%   -0.25%     
==========================================
  Files          47       47              
  Lines        2171     2179       +8     
==========================================
+ Hits         1968     1970       +2     
- Misses        203      209       +6     
Impacted Files Coverage Δ
R/zzz.R 0.00% <0.00%> (ø)
R/testing.R 75.43% <54.54%> (-3.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b94f31...caada74. Read the comment docs.

@lorenzwalthert
Copy link
Collaborator

The reason for the testing system to be implemented in its current form is that we want to leverage git diff to easily inspect the changes that happen to the *-out.R files when we break the tests. This is only possible if we overwrite the expected result of the test in the test directory.

I believe there is generally no problem with expecting the test directory to be writable, at least we have never experienced it. Do you have a particular use case in mind? Because in general, I don't think we should change this.

@michaelquinn32
Copy link
Contributor Author

The test directory not writeable at Google, and the change is one that we need to maintain to keep styler tested with our CI system. (FWIW, the other is that we don't allow changes to the working directory during tests; it's a much harder fix). We do this to because all runtime files are read-only, which is part of ensuring that tests are self-contained.

Using git diff --noindex lets you compare two files without having to fall back to the version control system.
https://git-scm.com/docs/git-diff

I know we've been bombarding you with issues recently (sorry). I don't know if anyone ever shared the context of all of this with you. Google's R community recently adopted the tidyverse style guide:
https://google.github.io/styleguide/Rguide.html

A key factor in that decision was styler, and being able to automatically correct style issues throughout our code base. For the last couple of weeks, we've been doing that, running the package over every R file that we have under version contol. In the future, something akin to a sytler precommit hook will make this automatic.

Anway, as far as this change goes, if it really impacts your workflow feel free to drop.

Thanks!

@lorenzwalthert
Copy link
Collaborator

Thanks for the context. Indeed we could use git diff in the temp dir, but the RStudio git Pane is much more convenient, so I think it should be the default. Also, I got notified about Google using styler and I must admit I am really thrilled about it. I am willing to find a solution that suits your needs. Did not connect you to this topic though 😄, now I understand better.

Ok, the rule you mention make sense now. If the tests pass, however, nothing should be written anywhere in the test directory, so I assume you just want to have more informative error messages for these cases. Two things come to mind: Environment variables and R options. We could introduce the R option styler.test_dir_unwritable (defaulting to FALSE), which, when TRUE, would run the tests in a temporary directory basically implementing your PR. The tricky thing is documenting these options or env variables somewhere. Would that be suitable for you?

@michaelquinn32
Copy link
Contributor Author

That would work for us.

For now, you don't have to worry about informative error messages. We can get around that as long as the tests are executable without modifications.

@lorenzwalthert
Copy link
Collaborator

For now, you don't have to worry about informative error messages. We can get around that as long as the tests are executable without modifications.

With informative error message, I meant that you want to see the testthat output, and not "Error: Directory xxx is not writable" (which I thought was why you need this PR).

Ok, so can you adapt your PR to:

  • introduce the aforementioned R option styler.test_dir_unwritable that governs whether test are executed in a temporary directory or not.
  • Set the option to FALSE in the .onLoad() function if not set already.

I will think about how to document this option (along with other ones we have that are not documented) as well as if we need to emit a message / warning during the test run if the option is set (just to make sure we don't forget about it).

Also note that with #538, there will be another problem with the tests/testthat directory being expected to be writable, but it seems this won't make it into upstream anytime soon. Maybe just activate notififcations for this PR and make sure you can remind me before we merge.

@lorenzwalthert
Copy link
Collaborator

I don't know if anyone ever shared the context of all of this with you. Google's R community recently adopted the tidyverse style guide: https://google.github.io/styleguide/Rguide.html

Maybe you could add a styler reference? I don't think the styler implementation of the tidyverse style guide would make code less compliant with your style guide.

@michaelquinn32
Copy link
Contributor Author

That's a great point. I'm on vacation now, but I'll add a reference when I get back.

michaelquinn32 added a commit to michaelquinn32/styleguide that referenced this pull request Jan 17, 2020
@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented May 1, 2020

Do you still want to have this merged? If yes, can you please adapt according to #548 (comment)? I am happy to merge afterwards.

@michaelquinn32
Copy link
Contributor Author

Sorry for the delay on this! I added an option for determining this behavior.

Thanks!

R/testing.R Outdated Show resolved Hide resolved
@lorenzwalthert
Copy link
Collaborator

Looks good. Can to add a NEWS.md section for this PR? Also, compared to #640, I wonder if we should set a default value in getOption() in general, even when we have the value set in .onLoad(). Not sure...

@michaelquinn32
Copy link
Contributor Author

Thanks for all of the feedback. You'll see from the commits that I took another pass here.

  1. I think having the default option is good; there are ways to unset the option if you're not careful.
  2. I changed the name to not be a negative. It makes it easier to reason about.
  3. There were some tests that try to write locally; set the flag there too.

@lorenzwalthert
Copy link
Collaborator

Cool, then should we merge this? I assume you tested everything to be working on Google's system as expected so we don't need another pass?

@michaelquinn32
Copy link
Contributor Author

Not unless I missed something. Let's merge

@lorenzwalthert lorenzwalthert merged commit 85a81fb into r-lib:master May 9, 2020
@lorenzwalthert
Copy link
Collaborator

Thanks @michaelquinn32.

@michaelquinn32 michaelquinn32 deleted the patch-1 branch May 13, 2020 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants