New meta-level test that roxygen2 has been run #2460

MichaelChirico · 2023-12-17T15:03:50Z

We've been bitten a number of times by some small change to the doc mark-up sneaking through to main without be roxygenize()'d, winding up included in some unrelated PR.

This simple workflow should prevent that issue from recurring.

codecov-commenter · 2023-12-17T15:06:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c4b8e34) 98.54% compared to head (4b8c9f7) 98.54%.

❗ Current head 4b8c9f7 differs from pull request most recent head 5cd9c07. Consider uploading reports for the commit 5cd9c07 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2460   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files         126      126           
  Lines        5721     5721           
=======================================
  Hits         5638     5638           
  Misses         83       83

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

MichaelChirico · 2023-12-17T15:07:59Z

Confirmed WAI:

AshesITR · 2023-12-17T15:35:59Z

Hmm this might be a problem for OS dependent Rd.
E.g. I can't generate the current man page for default_undesirable_functions without changing sort order.

MichaelChirico · 2023-12-17T15:37:11Z

even after #2455?

AshesITR · 2023-12-17T15:41:18Z

Should be fixed by that but a similar problem could occur in the future.
So if we put it in place, it should run at least on linux + windows + linux with non-standard locale to catch any potentially problematic markup ahead of time.

MichaelChirico · 2023-12-17T16:07:09Z

Should be fixed by that but a similar problem could occur in the future. So if we put it in place, it should run at least on linux + windows + linux with non-standard locale to catch any potentially problematic markup ahead of time.

IMO that will catch some rare (but annoying!) issues, but the current script will catch most issues. Not sure it's worth extra compute resources for the rest.

AshesITR · 2023-12-17T17:14:49Z

How much resources does it cost to run?
Maybe run it twice in the same process with different locales as a compromise?

I'm willing to give it a try (it won't be blocking anyway, so might just be a minor annoyance if encountered).

Perhaps output a git diff of old vs. new for changed files to ease debugging?

MichaelChirico · 2023-12-17T23:20:43Z

How much resources does it cost to run? Maybe run it twice in the same process with different locales as a compromise?

different locales is a good idea, that'll have low overhead. setting up different containers/OS was my issue.

Perhaps output a git diff of old vs. new for changed files to ease debugging?

I pondered this but figured it'd be pretty clear why it fails (oops, I forgot to document). let me add a one liner anyway.

AshesITR · 2023-12-18T04:50:23Z

Perhaps output a git diff of old vs. new for changed files to ease debugging?

I pondered this but figured it'd be pretty clear why it fails (oops, I forgot to document). let me add a one liner anyway.

Usually, yes. But if and when devtools::document() won't fix the run, it will be very frustrating to debug.

AshesITR · 2023-12-18T04:52:33Z

.dev/roxygen_test.R

+    }
+    cat(sprintf("roxygenize() output differs from saved output for %s.\n", file))
+    cat("Here's the Rd2txt() output for what's saved:\n")
+    Rd2txt(old_file)


Will this show macros etc.?

Per the help, the default is the "render" stage which expands macros:

https://rdrr.io/r/tools/Rd2HTML.html

IMO the raw contents of the Rd file are more helpful since those are the files that are checked in.

tools::Rdiff() will compare the raw contents and show the diff, here's the output when I changed <- to = in assignment_linter.Rd:

34c34 < code_lines <- "1 -> x\n2 ->> y" --- > code_lines = "1 -> x\n2 ->> y"

.dev/roxygen_test.R

AshesITR · 2023-12-19T06:54:54Z

.dev/roxygen_test.R

-    cat("Here's the Rd2txt() output for roxygenize():\n")
-    Rd2txt(new_file)
+    cat("Here's the tools::Rdiff() comparison of the two files:\n")
+    tools::Rdiff(old_file, new_file)


tmp1 <- withr::local_tempfile(lines = c("a", "b")) tmp2 <- withr::local_tempfile(lines = c("a", "b", "c")) tools::Rdiff(tmp1, tmp2) #> files differ in number of lines

^{Created on 2023-12-19 with reprex v2.0.2}

That's not very helpful, either...

Ah, that's annoying, thanks for checking. Maybe we should use testthat::expect_identical() to handle giving a summarized diff instead?

What about

tmp1 <- withr::local_tempfile(lines = c("a", "b")) tmp2 <- withr::local_tempfile(lines = c("a", "b", "c")) cat(capture.output(system2("diff", c("--unified", normalizePath(tmp1), normalizePath(tmp2)))), sep = "\n")

^{Created on 2023-12-19 with reprex v2.0.2}

tmp1 <- withr::local_tempfile(lines = c("a", "b")) tmp2 <- withr::local_tempfile(lines = c("a", "b", "c")) tools::Rdiff(tmp1, tmp2) #> files differ in number of lines

Created on 2023-12-19 with reprex v2.0.2

That's not very helpful, either...

Hmm, this truncated the output:

files differ in number of lines: 2a3 > c

Note that tools::Rdiff() is calling diff under the hood, just with different options (-bw vs. your --unified):

https://github.com/r-devel/r-svn/blob/c786526c27271c4bb04b621d2bde5cdd93033bc0/src/library/tools/R/testing.R#L290-L293

Vs. testthat:

testthat::expect_identical(readLines(tmp1), readLines(tmp2)) > Error: readLines(tmp1) (`actual`) not identical to readLines(tmp2) (`expected`). `actual`: "a" "b" `expected`: "a" "b" "c"

(with color if we enable it)

Oh, weird. I don't get the output from Rdiff in either my IDE or reprex.
I prefer --unified because that's basically git diff, which we're very used to reading.

Let's try it by forcing a failure?

Indeed plain Rdiff() output is junk:

Trying with Log=TRUE now.

OK, --unified looks like the way to go. One last small bit is to try and make this part comprehensible:

I wouldn't worry too much about that, unless there is a very simple option for diff to fake this path.

more importantly we need to make clear which output comes from which source. handled already at current commit.

AshesITR

NB cli is available if we want prettier messages.

.dev/roxygen_test.R

MichaelChirico added 2 commits December 17, 2023 15:01

new meta-level test that roxygen2 has been run

85ed38a

comparable->normalize

247a430

restore test diff

ff3220f

Run in several locales, print nicer failure

331f31a

AshesITR reviewed Dec 18, 2023

View reviewed changes

.dev/roxygen_test.R Outdated Show resolved Hide resolved

MichaelChirico added 2 commits December 17, 2023 21:20

check eats LOCALE

2f3150f

Use Rdiff to show the difference

9c777e5

AshesITR reviewed Dec 19, 2023

View reviewed changes

MichaelChirico and others added 8 commits December 21, 2023 07:49

Update any_is_na_linter.R

f2d345c

try Log=TRUE

43589ce

bad copy-paste

0b6a161

diff --unified

222c53e

annotate output

a24c21b

Simplify

9c5b5dc

Merge branch 'main' into roxy-action

f799491

Merge branch 'main' into roxy-action

c9ae7bd

AshesITR reviewed Dec 23, 2023

View reviewed changes

.dev/roxygen_test.R Outdated Show resolved Hide resolved

.dev/roxygen_test.R Show resolved Hide resolved

Update roxygen_test.R

5cd9c07

AshesITR approved these changes Dec 23, 2023

View reviewed changes

AshesITR merged commit acbfd77 into main Dec 23, 2023
20 checks passed

MichaelChirico deleted the roxy-action branch December 27, 2023 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New meta-level test that roxygen2 has been run #2460

New meta-level test that roxygen2 has been run #2460

MichaelChirico commented Dec 17, 2023

codecov-commenter commented Dec 17, 2023 •

edited

Loading

MichaelChirico commented Dec 17, 2023

AshesITR commented Dec 17, 2023

MichaelChirico commented Dec 17, 2023

AshesITR commented Dec 17, 2023

MichaelChirico commented Dec 17, 2023

AshesITR commented Dec 17, 2023

MichaelChirico commented Dec 17, 2023

AshesITR commented Dec 18, 2023

AshesITR Dec 18, 2023

MichaelChirico Dec 18, 2023

AshesITR Dec 18, 2023

MichaelChirico Dec 19, 2023

AshesITR Dec 19, 2023

MichaelChirico Dec 19, 2023

AshesITR Dec 19, 2023 •

edited

Loading

MichaelChirico Dec 19, 2023

AshesITR Dec 19, 2023

AshesITR Dec 20, 2023

MichaelChirico Dec 21, 2023

MichaelChirico Dec 21, 2023

AshesITR Dec 22, 2023

MichaelChirico Dec 23, 2023

AshesITR left a comment

New meta-level test that roxygen2 has been run #2460

New meta-level test that roxygen2 has been run #2460

Conversation

MichaelChirico commented Dec 17, 2023

codecov-commenter commented Dec 17, 2023 • edited Loading

Codecov Report

MichaelChirico commented Dec 17, 2023

AshesITR commented Dec 17, 2023

MichaelChirico commented Dec 17, 2023

AshesITR commented Dec 17, 2023

MichaelChirico commented Dec 17, 2023

AshesITR commented Dec 17, 2023

MichaelChirico commented Dec 17, 2023

AshesITR commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshesITR Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshesITR left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 17, 2023 •

edited

Loading

AshesITR Dec 19, 2023 •

edited

Loading