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

Minimize output noise when R has no NLS support #1662

Merged
merged 7 commits into from
Sep 21, 2022

Conversation

dgkf
Copy link
Contributor

@dgkf dgkf commented Jul 30, 2022

Closes #1566

A pretty minor change to implement the suggested changes in #1566.

I made one additional design choice to emit a once-per-session warning when local_reproducible_output is run without NLS, but I'm not sure exactly what the best behavior would be. I could see the argument either way - either as a good reminder that the tests may still fail on another system, or as an unfortunate state-dependent test result. I would have no problem removing it if reproducibility of test result warnings is the more important behavior.

When run without NLS capability, the first test to check against local_reproducible_output will emit a warning:

───────────────────────────────────────────────────────────
Warning (test-base.R:1:1): (code run outside of `test_that()`)
Local reproducible output can't be checked fully because R is installed without NLS
This warning is displayed once per session.
Backtrace:
 1. testthat::test_that(...)
      at test-base.R:1:0
 2. testthat::local_test_context()
      at testthat/R/test-that.R:40:2
 3. testthat::local_reproducible_output(.env = .env)
      at testthat/R/local.R:70:4
───────────────────────────────────────────────────────────

@dgkf
Copy link
Contributor Author

dgkf commented Jul 30, 2022

This issue has been annoying me for a while, so when I saw that the suggested changes were pretty minimal I just went ahead and fixed it locally. It was just a little extra work to prep it to share. Happy to make whatever changes are needed to get this issue resolved.

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.

Thanks for working on this! As well as the suggestion below, can you please also add a bullet to the top of NEWS.md? It should briefly describe the change and end with (@yourname, #issuenumber).

R/local.R Outdated Show resolved Hide resolved
@hadley hadley merged commit 8b4c265 into r-lib:main Sep 21, 2022
@hadley
Copy link
Member

hadley commented Sep 21, 2022

Thanks! I ended up removing the test because I don't think it actually tests the case we really care about, and hence just gives a false sense of security.

@dgkf dgkf deleted the 1566-local-rep-wo-nls branch September 21, 2022 20:30
heavywatal added a commit to heavywatal/testthat that referenced this pull request Sep 21, 2022
hadley pushed a commit that referenced this pull request Sep 22, 2022
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.

Warning: 'Changing language has no effect when R installed without NLS'
2 participants