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

Start using testthat 3e #910

Merged
merged 11 commits into from
Mar 11, 2022
Merged

Start using testthat 3e #910

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8f5d3a0
Start using testthat 3e
MichaelChirico Mar 4, 2022
e365f17
get migrated tests passing
MichaelChirico Mar 9, 2022
37ad97f
Merge branch 'master' into MichaelChirico-patch-2
MichaelChirico Mar 9, 2022
2b71ca7
reference PR
MichaelChirico Mar 9, 2022
24c5ff0
infix spacing
MichaelChirico Mar 10, 2022
0ec8ba4
add & use a helper for comparing environments
MichaelChirico Mar 10, 2022
8d9b0d6
Merge branch 'MichaelChirico-patch-2' of github.com:jimhester/lintr i…
MichaelChirico Mar 10, 2022
a54770c
add rlang Suggested
MichaelChirico Mar 10, 2022
21d64a8
revert custom helper
MichaelChirico Mar 11, 2022
c8e1490
remove rlang too
MichaelChirico Mar 11, 2022
243d03e
Merge branch 'master' into MichaelChirico-patch-2
AshesITR Mar 11, 2022
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
3 changes: 2 additions & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,13 @@ Suggests:
mockery,
rmarkdown,
rstudioapi (>= 0.2),
testthat (>= 2.2.1),
testthat (>= 3.0.0),
withr
License: MIT + file LICENSE
Encoding: UTF-8
VignetteBuilder: knitr
RoxygenNote: 7.1.2
Config/testthat/edition: 3
Collate:
'T_and_F_symbol_linter.R'
'utils.R'
Expand Down
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ function calls. (#850, #851, @renkun-ken)
* Each linter can have multiple tags
* New function `available_linters()` to list available linters and their tags
This feature is extensible by package authors providing add-on linters for {lintr}.
* `lintr` now uses the 3rd edition of `testthat` (@MichaelChirico)
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved

# lintr 2.0.1

Expand Down
158 changes: 65 additions & 93 deletions tests/testthat/test-cache.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,116 +53,92 @@ test_that("clear_cache deletes the directory if no file is given", {
# `load_cache`

test_that("load_cache loads the saved file in a new empty environment", {
with_mock(
`lintr::read_settings` = function(...) invisible(...),

e1 <- new.env(parent = emptyenv()),
assign("x", "foobar", envir = e1),
d1 <- tempfile(pattern = "lintr_cache_"),
f1 <- "R/test.R",
save_cache(cache = e1, file = f1, path = d1),
e2 <- load_cache(file = f1, path = d1),

expect_equal(ls(e2), "x"),
expect_equal(e2[["x"]], "foobar")
)
e1 <- new.env(parent = emptyenv())
assign("x", "foobar", envir = e1)
d1 <- tempfile(pattern = "lintr_cache_")
f1 <- "R/test.R"
save_cache(cache = e1, file = f1, path = d1)
e2 <- load_cache(file = f1, path = d1)

expect_equal(ls(e2), "x")
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
expect_equal(e2[["x"]], "foobar")
})

test_that("load_cache returns an empty environment if no cache file exists", {
with_mock(
`lintr::read_settings` = function(...) invisible(...),

e1 <- new.env(parent = emptyenv()),
d1 <- tempfile(pattern = "lintr_cache_"),
f1 <- "R/test.R",
f2 <- "test.R",
e1 <- new.env(parent = emptyenv())
d1 <- tempfile(pattern = "lintr_cache_")
f1 <- "R/test.R"
f2 <- "test.R"

save_cache(cache = e1, file = f1, path = d1),
e2 <- load_cache(file = f2, path = d1),
save_cache(cache = e1, file = f1, path = d1)
e2 <- load_cache(file = f2, path = d1)

expect_equal(ls(e2), character(0))
)
expect_equal(ls(e2), character(0))
})

test_that("load_cache returns an empty environment if reading cache file fails", {
with_mock(
`lintr::read_settings` = function(...) invisible(...),

e1 <- new.env(parent = emptyenv()),
assign("x", "foobar", envir = e1),
d1 <- tempfile(pattern = "lintr_cache_"),
f1 <- "R/test.R",
save_cache(cache = e1, file = f1, path = d1),
cache_f1 <- file.path(d1, fhash(f1)),
writeLines(character(), cache_f1),
expect_warning(e2 <- load_cache(file = f1, path = d1)),
saveRDS(e1, cache_f1),
expect_warning(e3 <- load_cache(file = f1, path = d1)),
expect_equal(ls(e2), character(0)),
expect_equal(ls(e3), character(0))
)
e1 <- new.env(parent = emptyenv())
assign("x", "foobar", envir = e1)
d1 <- tempfile(pattern = "lintr_cache_")
f1 <- "R/test.R"
save_cache(cache = e1, file = f1, path = d1)
cache_f1 <- file.path(d1, fhash(f1))
writeLines(character(), cache_f1)
expect_warning(e2 <- load_cache(file = f1, path = d1))
saveRDS(e1, cache_f1)
expect_warning(e3 <- load_cache(file = f1, path = d1))
expect_equal(ls(e2), character(0))
expect_equal(ls(e3), character(0))
})

# `save_cache`

test_that("save_cache creates a directory if needed", {
with_mock(
`lintr::read_settings` = function(...) invisible(...),

e1 <- new.env(parent = emptyenv()),
d1 <- tempfile(pattern = "lintr_cache_"),
f1 <- "R/test.R",
e1 <- new.env(parent = emptyenv())
d1 <- tempfile(pattern = "lintr_cache_")
f1 <- "R/test.R"

expect_false(file.exists(d1)),
expect_false(file.exists(file.path(d1, fhash(f1)))),
expect_false(file.exists(d1))
expect_false(file.exists(file.path(d1, fhash(f1))))

save_cache(cache = e1, file = f1, path = d1),
save_cache(cache = e1, file = f1, path = d1)

expect_true(file.exists(d1)),
expect_true(file.info(d1)$isdir),
expect_true(file.exists(file.path(d1, fhash(f1))))
)
expect_true(file.exists(d1))
expect_true(file.info(d1)$isdir)
expect_true(file.exists(file.path(d1, fhash(f1))))
})

test_that("save_cache uses unambiguous cache file names", {
with_mock(
`lintr::read_settings` = function(...) invisible(...),

e1 <- new.env(parent = emptyenv()),
d1 <- tempfile(pattern = "lintr_cache_"),
f1 <- "R/test.R",
f2 <- "test.R",
e1 <- new.env(parent = emptyenv())
d1 <- tempfile(pattern = "lintr_cache_")
f1 <- "R/test.R"
f2 <- "test.R"

expect_false(file.exists(file.path(d1, fhash(f1)))),
expect_false(file.exists(file.path(d1, fhash(f2)))),
expect_false(file.exists(file.path(d1, fhash(f1))))
expect_false(file.exists(file.path(d1, fhash(f2))))

save_cache(cache = e1, file = f1, path = d1),
save_cache(cache = e1, file = f2, path = d1),
save_cache(cache = e1, file = f1, path = d1)
save_cache(cache = e1, file = f2, path = d1)

expect_true(fhash(f1) != fhash(f2)),
expect_true(file.exists(file.path(d1, fhash(f1)))),
expect_true(file.exists(file.path(d1, fhash(f2))))
)
expect_true(fhash(f1) != fhash(f2))
expect_true(file.exists(file.path(d1, fhash(f1))))
expect_true(file.exists(file.path(d1, fhash(f2))))
})

test_that("save_cache saves all non-hidden objects from the environment", {
with_mock(
`lintr::read_settings` = function(...) invisible(...),

e1 <- new.env(parent = emptyenv()),
e1$t1 <- 1,
e1$t2 <- 2,
e1 <- new.env(parent = emptyenv())
e1$t1 <- 1
e1$t2 <- 2

d1 <- tempfile(pattern = "lintr_cache_"),
f1 <- "R/test.R",
d1 <- tempfile(pattern = "lintr_cache_")
f1 <- "R/test.R"

save_cache(cache = e1, file = f1, path = d1),
save_cache(cache = e1, file = f1, path = d1)

e2 <- new.env(parent = emptyenv()),
load(file = file.path(d1, fhash(f1)), envir = e2),
e2 <- new.env(parent = emptyenv())
load(file = file.path(d1, fhash(f1)), envir = e2)

expect_equal(e1, e2)
)
expect_identical(as.list(e1), as.list(e2))
Copy link
Member

Choose a reason for hiding this comment

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

If you use this approach, I think you need all.names = TRUE to verify the property implied by the test title.

OTOH I'm not sure what "non-hidden" means as I would have expected hidden to imply a variable starting with ., but that doesn't appear in the test.

Additionally, I'd use expect_equal() here because expect_identical() just implies exact numerical comparisons, which I don't think is important here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's not my reading of the docs (?expect_identical):

expect_identical() is the baseline; expect_equal() relaxes the test to ignore small numeric differences.

i.e., only use expect_equal to ignore numerical differences

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm. I guess it missing another sentence that says something like: "Since in most cases tiny numeric differences aren't important, we recommend using expect_equal()."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🙈 our internal policy encourages expect_identical() now and we've basically switched all the expect_equal() tests already, except where ignore*= or tolerance= are needed. I have come to quite like it TBH. It encourages stronger tests.

})

# `cache_file`
Expand Down Expand Up @@ -435,18 +411,14 @@ test_that("lint with cache uses the provided relative cache directory", {
test_that("it works outside of a package", {
linter <- assignment_linter()

with_mock(
`lintr::find_package` = function(...) NULL,
`lintr::pkg_name` = function(...) NULL,

path <- tempfile(pattern = "my_cache_dir_"),
expect_false(dir.exists(path)),
expect_lint("a <- 1", NULL, linter, cache = path),
expect_true(dir.exists(path)),
expect_length(list.files(path), 1),
expect_lint("a <- 1", NULL, linter, cache = path),
expect_true(dir.exists(path))
)
mockery::stub(lintr:::find_default_encoding, "find_package", function(...) NULL)
AshesITR marked this conversation as resolved.
Show resolved Hide resolved
path <- tempfile(pattern = "my_cache_dir_")
expect_false(dir.exists(path))
expect_lint("a <- 1", NULL, linter, cache = path)
expect_true(dir.exists(path))
expect_length(list.files(path), 1)
expect_lint("a <- 1", NULL, linter, cache = path)
expect_true(dir.exists(path))
})

test_that("cache = TRUE workflow works", {
Expand Down
78 changes: 33 additions & 45 deletions tests/testthat/test-ci.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,67 +1,55 @@
test_that("GitHub Actions functionality works", {
# imitate being on GHA whether or not we are
withr::with_envvar(c(GITHUB_ACTIONS = "true"), {
old <- options(lintr.rstudio_source_markers = FALSE)
on.exit(options(old), add = TRUE)
withr::local_envvar(list(GITHUB_ACTIONS = "true"))
withr::local_options(lintr.rstudio_source_markers = FALSE)
tmp <- withr::local_tempfile()

writeLines("x <- 1:nrow(y)", tmp <- tempfile())
on.exit(unlink(tmp))
writeLines("x <- 1:nrow(y)", tmp)

l <- lint(tmp)
expect_output(print(l), "::warning file", fixed = TRUE)
})
l <- lint(tmp)
expect_output(print(l), "::warning file", fixed = TRUE)
})

test_that("GitHub Actions functionality works in a subdirectory", {
# imitate being on GHA whether or not we are
pkg_path <- file.path("dummy_packages", "assignmentLinter")
withr::with_envvar(c(GITHUB_ACTIONS = "true"), {
old <- options(
lintr.rstudio_source_markers = FALSE,
lintr.github_annotation_project_dir = pkg_path
)
on.exit(options(old), add = TRUE)

read_settings(NULL)
l <- lint_package(
pkg_path, linters = list(assignment_linter()),
parse_settings = FALSE
)
expect_output(
print(l),
paste0("::warning file=", file.path(pkg_path, "R(/|\\\\)abc\\.R"))
)
})
withr::local_envvar(list(GITHUB_ACTIONS = "true"))
withr::local_options(lintr.rstudio_source_markers = FALSE, lintr.github_annotation_project_dir = pkg_path)

read_settings(NULL)
l <- lint_package(
pkg_path,
linters = list(assignment_linter()),
parse_settings = FALSE
)
expect_output(
print(l),
paste0("::warning file=", file.path(pkg_path, "R(/|\\\\)abc\\.R"))
)
})

test_that("Printing works for Travis", {
withr::with_envvar(c(GITHUB_ACTIONS = "false", TRAVIS_REPO_SLUG = "test/repo", LINTR_COMMENT_BOT = "true"), {
old <- options(lintr.rstudio_source_markers = FALSE)
on.exit(options(old), add = TRUE)
withr::local_envvar(list(GITHUB_ACTIONS = "false", TRAVIS_REPO_SLUG = "test/repo", LINTR_COMMENT_BOT = "true"))
withr::local_options(lintr.rstudio_source_markers = FALSE)
tmp <- withr::local_tempfile()
writeLines("x <- 1:nrow(y)", tmp)

writeLines("x <- 1:nrow(y)", tmp <- tempfile())
on.exit(unlink(tmp))
l <- lint(tmp)

l <- lint(tmp)

with_mock(github_comment = function(x, ...) cat(x, "\n"), .env = asNamespace("lintr"), {
expect_output(print(l), "*warning:*", fixed = TRUE)
})
with_mock(github_comment = function(x, ...) cat(x, "\n"), .env = asNamespace("lintr"), {
expect_output(print(l), "*warning:*", fixed = TRUE)
})
})

test_that("Printing works for Wercker", {
withr::with_envvar(c(GITHUB_ACTIONS = "false", WERCKER_GIT_BRANCH = "test/repo", LINTR_COMMENT_BOT = "true"), {
old <- options(lintr.rstudio_source_markers = FALSE)
on.exit(options(old), add = TRUE)

writeLines("x <- 1:nrow(y)", tmp <- tempfile())
on.exit(unlink(tmp))
withr::local_envvar(list(GITHUB_ACTIONS = "false", WERCKER_GIT_BRANCH = "test/repo", LINTR_COMMENT_BOT = "true"))
withr::local_options(lintr.rstudio_source_markers = FALSE)
tmp <- withr::local_tempfile()
writeLines("x <- 1:nrow(y)", tmp)

l <- lint(tmp)
l <- lint(tmp)

with_mock(github_comment = function(x, ...) cat(x, "\n"), .env = asNamespace("lintr"), {
expect_output(print(l), "*warning:*", fixed = TRUE)
})
with_mock(github_comment = function(x, ...) cat(x, "\n"), .env = asNamespace("lintr"), {
expect_output(print(l), "*warning:*", fixed = TRUE)
})
})
2 changes: 1 addition & 1 deletion tests/testthat/test-commented_code_linter.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
test_that("returns the correct linting", {
msg <- rex("Commented code should be removed.")
linter <- commented_code_linter()
expect_is(linter, "linter")
expect_s3_class(linter, "linter")

expect_lint("blah", NULL, linter)

Expand Down
6 changes: 3 additions & 3 deletions tests/testthat/test-defaults.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
test_that("linters", {
# non-empty named list of functions
x <- default_linters
expect_is(x, "list")
expect_type(x, "list")
expect_gt(length(x), 0L)
expect_true(all(names(x) != ""))
expect_true(all(vapply(x, inherits, logical(1L), "linter")))
Expand All @@ -14,7 +14,7 @@ test_that("undesirable functions and operators", {
all_undesirable_operators, default_undesirable_operators)

for (x in vars) {
expect_is(x, "list")
expect_type(x, "list")
expect_gt(length(x), 0L)
expect_true(all(names(x) != ""))
expect_true(all(vapply(x, function(x) is.na(x) || is.character(x), logical(1L))))
Expand All @@ -25,7 +25,7 @@ test_that("undesirable functions and operators", {
test_that("settings", {
# non-empty named list
x <- default_settings
expect_is(x, "list")
expect_type(x, "list")
expect_gt(length(x), 0L)
expect_true(all(names(x) != ""))
})
Expand Down
Loading