Upgrade to testthat 3e #949

IndrajeetPatil · 2022-05-26T09:03:09Z

Closes #783

TODO:

Remove context() statements using-

xfun::gsub_dir(
  'context\\(.+\\)',
  '',
  ext = "R",
  dir = "tests/"
)

expect_equivalent() -> expect_equal()
Replace expect_known_value() with expect_snapshot_value()
Delete unnecessary binary files
~~Use expect_snapshot_error(...) instead of expect_error(..., regexp)~~
~~Use expect_snapshot_warning(...) instead of expect_warning(..., regexp)~~
~~Use expect_snapshot_message(...) instead of expect_message(..., regexp)~~

Closes #783

github-actions · 2022-05-26T10:24:36Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2af428b is merged into main:

:ballot_box_with_check:cache_applying: 32.2ms -> 32.5ms [-1.81%, +3.55%]
:ballot_box_with_check:cache_recording: 1.55s -> 1.55s [-1.14%, +1.13%]
:ballot_box_with_check:without_cache: 4.15s -> 4.14s [-1.22%, +0.83%]

Further explanation regarding interpretation and methodology can be found in the documentation.

lorenzwalthert · 2022-05-26T11:39:03Z

You are on a roll today 🥳. All checks green so far. Also, some checks in test-public_api.R (or similar) are checking for printed output too, maybe they should also use snapshots instead. Did you see https://www.tidyverse.org/blog/2022/02/upkeep-testthat-3/ ?

lorenzwalthert · 2022-05-26T11:40:24Z

Also note that some utf8 related tests give a different Output for me locally, but it passes CI and CRAN, so I suggest to not touch them.

IndrajeetPatil · 2022-05-28T10:18:33Z

maybe they should also use snapshots instead

Yes, I am now using snapshots for the relevant tests. Let's see if the tests pass now.

If I understand the previous testing infrastructure correctly, with a switch to snapshots, the binary files here should be deleted, right?

codecov-commenter · 2022-05-28T10:20:37Z

Codecov Report

Merging #949 (ce61f69) into main (08301c0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #949   +/-   ##
=======================================
  Coverage   89.81%   89.81%           
=======================================
  Files          47       47           
  Lines        2611     2611           
=======================================
  Hits         2345     2345           
  Misses        266      266

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 08301c0...ce61f69. Read the comment docs.

IndrajeetPatil · 2022-05-28T10:27:01Z

@lorenzwalthert The following code:

 cache_info[, c("n", "size", "last_modified", "activated")]

produces the following output on R versions > 3.6

      # A tibble: 1 x 4
            n  size last_modified activated
        <int> <dbl> <dttm>        <lgl>    
      1     0     0 -Inf -Inf     FALSE

and otherwise

      # A tibble: 1 x 4
            n  size last_modified activated
        <int> <dbl> <dttm>        <lgl>    
      1     0     0 NA NA         FALSE

Should this test be skipped on older versions?

lorenzwalthert · 2022-05-28T11:56:48Z

Yes. That one can be skipped on old R versions. And the binary (but not the R files). can be deleted in that dir.

github-actions · 2022-05-28T13:18:49Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if c963258 is merged into main:

:ballot_box_with_check:cache_applying: 26.1ms -> 26.2ms [-0.67%, +1.85%]
:ballot_box_with_check:cache_recording: 1.13s -> 1.13s [-0.82%, +0.45%]
:ballot_box_with_check:without_cache: 3.06s -> 3.06s [-0.59%, +0.18%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert

@lorenzwalthert Can you please check if this is what you want? More specifically, do you want to include snaphots of entire error messages or are you happy with the current approach of using regexps to check error messages?

lorenzwalthert · 2022-05-30T20:55:50Z

@IndrajeetPatil about the errors: I am not sure we want to catch exact errors that are defined outside {styler}. In the past, R core changed the parser and I had to release a new styler version just to fix these. I am happy snapshotting our own errors but probably it's better to rely on regex for f1d87bd and the like. Is there a way to do that with the third edition?

github-actions · 2022-05-30T21:50:54Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if a241065 is merged into main:

:ballot_box_with_check:cache_applying: 34.1ms -> 33.3ms [-4.94%, +0.38%]
:ballot_box_with_check:cache_recording: 1.62s -> 1.62s [-0.73%, +1.02%]
:ballot_box_with_check:without_cache: 4.41s -> 4.4s [-1.18%, +0.76%]

Further explanation regarding interpretation and methodology can be found in the documentation.

MichaelChirico · 2022-05-31T03:13:53Z

@IndrajeetPatil about the errors: I am not sure we want to catch exact errors that are defined outside {styler}. In the past, R core changed the parser and I had to release a new styler version just to fix these. I am happy snapshotting our own errors but probably it's better to rely on regex for f1d87bd and the like. Is there a way to do that with the third edition?

You might also consider generating the base error message yourself and comparing to that -- should be even more stable than regexes. data.table does this -- see the get_msg() helper in our main test suite: https://raw.githubusercontent.com/Rdatatable/data.table/master/inst/tests/tests.Rraw

This reverts commit f1d87bd.

lorenzwalthert · 2022-05-31T06:22:24Z

Thanks @MichaelChirico. But what's the advantage over expect_error() in that context. Since expect_error() is not (soft-)deprecated or superseded, I think we should just continue to use it for those type of non-styler messages.

IndrajeetPatil · 2022-05-31T06:23:26Z

but probably it's better to rely on regex for f1d87bd and the like. Is there a way to do that with the third edition?

Yeah, the third edition has no issues with using regex for testing error messages. So basically nothing needs to be changed with these tests.

In this case, there is nothing more that needs to be done for this PR.

IndrajeetPatil · 2022-05-31T06:56:06Z

@lorenzwalthert I am not sure why pre-commit GHA is failing now. Seems to do with end of files, but not sure how to fix this:

github-actions · 2022-05-31T07:32:49Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 06241bd is merged into main:

:ballot_box_with_check:cache_applying: 25.7ms -> 25.6ms [-1.15%, +0.82%]
:ballot_box_with_check:cache_recording: 1.09s -> 1.1s [-0.18%, +1.29%]
:ballot_box_with_check:without_cache: 2.9s -> 2.92s [-1.17%, +2.95%]

Further explanation regarding interpretation and methodology can be found in the documentation.

lorenzwalthert · 2022-05-31T08:09:00Z

Well there is some important definition / convention that a file should end with one and exactly one line break. That's enforced with that hook, so here are the options to fix it:

remove the redundant lines manually.
install pre-commit locally and run pre-commit run (makes sense if you want to use that framework in the future)
file an issue with testthat that every snapshot should contain one and exactly one empty line at EOF. That of course would be a breaking chnage to testthat snaps, but I think it's the right thing to do. Do you want to file an issue or should I?

IndrajeetPatil · 2022-05-31T09:25:45Z

I am not sure if this is a testthat issue.

This happens because the code output itself contains a line break, and then testthat adds another line break.

As a result, utils.md contains two line breaks:

IndrajeetPatil · 2022-05-31T09:28:03Z

The easiest and quickest solution here is to remove the extra line break manually.

This reverts commit 7c2db48.

lorenzwalthert · 2022-05-31T09:44:21Z

But then whenever you run testthat, it will add it, no? In that case, maybe we should exclude the snaps from pre-commit...

IndrajeetPatil · 2022-05-31T09:56:20Z

maybe we should exclude the snaps from pre-commit...

Indeed, that seems like the best way to go.

Sorry, not too familiar with the precommit framework. How can I exclude the .md files in tests/testthat/_snaps folder?

lorenzwalthert · 2022-05-31T10:17:30Z

Adapt .pre-commit-config.yaml like this

    -   id: end-of-file-fixer
        exclude: >
          (?x)^(
          \.Rd|
          tests/testthat/exception_handling/empty_file\.R|
          tests/testthat/parse_comments/eol_eof_spaces-.*|
          tests/testthat/reference-objects/.*|
          tests/testthat/_snaps/.*|
          )$

github-actions · 2022-05-31T10:26:23Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if b38acf5 is merged into main:

:ballot_box_with_check:cache_applying: 22.9ms -> 23.3ms [-1.29%, +4.43%]
:ballot_box_with_check:cache_recording: 970ms -> 971ms [-0.35%, +0.59%]
:ballot_box_with_check:without_cache: 2.6s -> 2.6s [-0.42%, +1.03%]

Further explanation regarding interpretation and methodology can be found in the documentation.

lorenzwalthert · 2022-05-31T10:47:21Z

Perfect @IndrajeetPatil. Thanks a lot. I also added a NEWS entry.

github-actions · 2022-05-31T11:47:54Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 19c5099 is merged into main:

:ballot_box_with_check:cache_applying: 27.7ms -> 27.6ms [-1.35%, +0.96%]
:ballot_box_with_check:cache_recording: 1.17s -> 1.17s [-1.08%, +0.48%]
:ballot_box_with_check:without_cache: 3.16s -> 3.17s [-0.15%, +0.51%]

Further explanation regarding interpretation and methodology can be found in the documentation.

IndrajeetPatil · 2022-05-31T12:27:15Z

Thanks, Lorenz!

Btw, precommit is really awesome, so thanks for that as well. Included it here:
https://github.com/IndrajeetPatil/awesome-r-pkgtools/blob/master/README.md

IndrajeetPatil added 2 commits May 26, 2022 11:02

Upgrade to testthat 3e

9cc9a8a

Closes #783

oops

0d95261

expect_known_value()-> expect_snapshot()

38d27e7

IndrajeetPatil added 3 commits May 28, 2022 12:37

concatenate outputs

a6a1c04

more snapshots

3291dfe

replace invisible with silent

ed208cd

IndrajeetPatil added 4 commits May 28, 2022 14:00

delete binaries; skip cache test on R < 3.6

c1bbae9

avoid negation

616e137

can run only on R4

6eccc0d

only 4.2 and onwards

ae46df8

use snapshots for error

f1d87bd

@lorenzwalthert Can you please check if this is what you want? More specifically, do you want to include snaphots of entire error messages or are you happy with the current approach of using regexps to check error messages?

IndrajeetPatil added 2 commits May 31, 2022 08:12

Revert "use snapshots for error"

2005860

This reverts commit f1d87bd.

revert some more

43377cd

IndrajeetPatil marked this pull request as ready for review May 31, 2022 06:21

IndrajeetPatil added 3 commits May 31, 2022 08:27

expect_equivalent() -> expect_equal()

2921c02

Update test-helpers.R

0ac2aef

Update test-public_api.R

9cea940

Update test-roundtrip.R

35d336f

IndrajeetPatil added 2 commits May 31, 2022 11:28

Update utils.md

7c2db48

Revert "Update utils.md"

844583c

This reverts commit 7c2db48.

IndrajeetPatil and others added 2 commits May 31, 2022 12:26

Update .pre-commit-config.yaml

ce61f69

add news

30cd025

lorenzwalthert merged commit 76a7421 into r-lib:main May 31, 2022

IndrajeetPatil deleted the 783_testthat3e branch May 31, 2022 11:49

IndrajeetPatil mentioned this pull request May 31, 2022

Upgrade to roxygen2 7.2.0 #950

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to testthat 3e #949

Upgrade to testthat 3e #949

IndrajeetPatil commented May 26, 2022 •

edited

Loading

github-actions bot commented May 26, 2022

lorenzwalthert commented May 26, 2022

lorenzwalthert commented May 26, 2022

IndrajeetPatil commented May 28, 2022

codecov-commenter commented May 28, 2022 •

edited

Loading

IndrajeetPatil commented May 28, 2022 •

edited

Loading

lorenzwalthert commented May 28, 2022

github-actions bot commented May 28, 2022

lorenzwalthert commented May 30, 2022 •

edited

Loading

github-actions bot commented May 30, 2022

MichaelChirico commented May 31, 2022

lorenzwalthert commented May 31, 2022

IndrajeetPatil commented May 31, 2022

IndrajeetPatil commented May 31, 2022

github-actions bot commented May 31, 2022

lorenzwalthert commented May 31, 2022 •

edited

Loading

IndrajeetPatil commented May 31, 2022

IndrajeetPatil commented May 31, 2022

lorenzwalthert commented May 31, 2022

IndrajeetPatil commented May 31, 2022

lorenzwalthert commented May 31, 2022 •

edited

Loading

github-actions bot commented May 31, 2022

lorenzwalthert commented May 31, 2022

github-actions bot commented May 31, 2022

IndrajeetPatil commented May 31, 2022

Upgrade to testthat 3e #949

Upgrade to testthat 3e #949

Conversation

IndrajeetPatil commented May 26, 2022 • edited Loading

github-actions bot commented May 26, 2022

lorenzwalthert commented May 26, 2022

lorenzwalthert commented May 26, 2022

IndrajeetPatil commented May 28, 2022

codecov-commenter commented May 28, 2022 • edited Loading

Codecov Report

IndrajeetPatil commented May 28, 2022 • edited Loading

lorenzwalthert commented May 28, 2022

github-actions bot commented May 28, 2022

lorenzwalthert commented May 30, 2022 • edited Loading

github-actions bot commented May 30, 2022

MichaelChirico commented May 31, 2022

lorenzwalthert commented May 31, 2022

IndrajeetPatil commented May 31, 2022

IndrajeetPatil commented May 31, 2022

github-actions bot commented May 31, 2022

lorenzwalthert commented May 31, 2022 • edited Loading

IndrajeetPatil commented May 31, 2022

IndrajeetPatil commented May 31, 2022

lorenzwalthert commented May 31, 2022

IndrajeetPatil commented May 31, 2022

lorenzwalthert commented May 31, 2022 • edited Loading

github-actions bot commented May 31, 2022

lorenzwalthert commented May 31, 2022

github-actions bot commented May 31, 2022

IndrajeetPatil commented May 31, 2022

IndrajeetPatil commented May 26, 2022 •

edited

Loading

codecov-commenter commented May 28, 2022 •

edited

Loading

IndrajeetPatil commented May 28, 2022 •

edited

Loading

lorenzwalthert commented May 30, 2022 •

edited

Loading

lorenzwalthert commented May 31, 2022 •

edited

Loading

lorenzwalthert commented May 31, 2022 •

edited

Loading