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.

Sign up for GitHub

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

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

Merged
merged 15 commits into from
Dec 23, 2023
Merged

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

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
85ed38a
new meta-level test that roxygen2 has been run
MichaelChirico Dec 17, 2023
247a430
comparable->normalize
MichaelChirico Dec 17, 2023
ff3220f
restore test diff
MichaelChirico Dec 17, 2023
331f31a
Run in several locales, print nicer failure
MichaelChirico Dec 18, 2023
2f3150f
check eats LOCALE
MichaelChirico Dec 18, 2023
9c777e5
Use Rdiff to show the difference
MichaelChirico Dec 19, 2023
f2d345c
Update any_is_na_linter.R
MichaelChirico Dec 20, 2023
43589ce
try Log=TRUE
MichaelChirico Dec 21, 2023
0b6a161
bad copy-paste
MichaelChirico Dec 21, 2023
222c53e
diff --unified
MichaelChirico Dec 21, 2023
a24c21b
annotate output
MichaelChirico Dec 21, 2023
9c5b5dc
Simplify
MichaelChirico Dec 21, 2023
f799491
Merge branch 'main' into roxy-action
MichaelChirico Dec 21, 2023
c9ae7bd
Merge branch 'main' into roxy-action
MichaelChirico Dec 23, 2023
5cd9c07
Update roxygen_test.R
MichaelChirico Dec 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions .dev/roxygen_test.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,8 @@ check_roxygenize_idempotent <- function(LOCALE) {
next
}
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)
cat("Here's the Rd2txt() output for roxygenize():\n")
Rd2txt(new_file)
cat("Here's the tools::Rdiff() comparison of the two files:\n")
AshesITR marked this conversation as resolved.
Show resolved Hide resolved
tools::Rdiff(old_file, new_file)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@AshesITR AshesITR Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try it by forcing a failure?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed plain Rdiff() output is junk:

image

Trying with Log=TRUE now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

stop("Failed in LOCALE=", LOCALE, ".", call. = FALSE)
AshesITR marked this conversation as resolved.
Show resolved Hide resolved
}
}
Expand Down
Loading