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

enable switching to the faster, time-stamp-based modification detection in auto_test() & auto_test_package() #598

Merged
merged 2 commits into from
Oct 2, 2017

Conversation

katrinleinweber
Copy link
Contributor

I haven't bench-marked the difference, just found it neat to have this option in the auto_test() functions, as well.

@katrinleinweber
Copy link
Contributor Author

katrinleinweber commented Jun 8, 2017

Travis says: LaTeX Error: File 'xkeyval.sty' not found. Since there are several other errors in other PRs, I'm not sure whether mine triggered this or not. devtools::check() on my machine does not produce any warnings or errors, but doesn't do "checking PDF version of manual".

@katrinleinweber katrinleinweber changed the title enable switching to the faster, time-stamp-based modification detection in auto_test(s) enable switching to the faster, time-stamp-based modification detection in auto_test() & auto_test_package() Jun 9, 2017
@katrinleinweber katrinleinweber force-pushed the disable-hashing-on-demand branch from b487b00 to ea55ac0 Compare June 9, 2017 14:28
In order to enable on-demand switching to the faster, time-stamp-based modification detection.
@katrinleinweber katrinleinweber force-pushed the disable-hashing-on-demand branch from ea55ac0 to 197bee3 Compare June 23, 2017 12:10
@katrinleinweber
Copy link
Contributor Author

Hello @krlmlr! There are currently no tests for the two functions I modified here. Would the recursion of testing auto_test_package() while testing testthat even be possible? 😉 Either way, I'm not sure how to fulfil the codecov target, sorry.

@krlmlr
Copy link
Member

krlmlr commented Jun 23, 2017

Thanks. I guess "user testing" is fine for this function.

Have you seen my brushthat package? I'd be very much interested in auto_test() functionality there, too!

@katrinleinweber
Copy link
Contributor Author

On my machine, auto_test_package() against testthat [c7e83308](https://github.com/hadley/testthat/tree/c7e8330867645c174f9a286d00eb0036cea78b0c) yields the same results as my modification auto_test_package(hash = FALSE): some mismatches in the reporters/..txt files.

I also ran the following user test. Is that a valid approach in your view?

  • in my macOS: checking /Library/Frameworks/R.framework/Versions/3.4/Resources/library for the largest dir => dplyr (well, 10 MB)
  • in Terminal: git clone https://github.com/tidyverse/dplyr.git --depth=1
  • in RStudio: uninstalled testthat and reinstalled with devtools::install_github("hadley/testthat")
    • in RStudio: associate new project with cloned dplyr folder & open
    • auto_test_package() => some skipped, none failed
    • back in testthat on my branch: Clean & Rebuild
    • back in dplyr: auto_test_package(hash = FALSE) => some skipped, none failed

This, together with the checks here seems to say: I regress nothing ;-D

I guess that to get more conclusive tests results, one would need to measure the reaction time of watch(hash = TRUE) vs. watch(hash = FALSE) in an even bigger package, but that is out-of-scope for me.

Please just consider this PR as enabling an existing, low-level option in the high-level functions as well.

@katrinleinweber
Copy link
Contributor Author

Hello @krlmlr!

[…] "user testing" is fine for this function.

Does that mean the codecov/patch check can be ignored and this PR will be merged? Kind regards!

@hadley hadley merged commit 8045af4 into r-lib:master Oct 2, 2017
@hadley
Copy link
Member

hadley commented Oct 2, 2017

Thanks!

@katrinleinweber
Copy link
Contributor Author

You're welcome :-)

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.

3 participants