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

Temporary coverage fix for #31 #32

Merged
merged 2 commits into from
Mar 16, 2024
Merged

Conversation

brownag
Copy link
Contributor

@brownag brownag commented Mar 15, 2024

This seems to get our codecov action working again, for #31, but I am not satisfied with this as a general solution. I do not understand at this time why it would matter if there was code in utils.R or not.

There seems to be some pretty odd stuff that can happen when target tests are being run inside testthat via covr.

@brownag brownag mentioned this pull request Mar 15, 2024
@Aariq
Copy link
Collaborator

Aariq commented Mar 15, 2024

The only thing I can think of is that the files in R/ are sourced in alphabetical order. But no idea why sourcing that function later would fix anything!

@Aariq
Copy link
Collaborator

Aariq commented Mar 15, 2024

I suppose you could check if putting it zzzz.R also "fixes" things to test that hypothesis.

@Aariq
Copy link
Collaborator

Aariq commented Mar 15, 2024

Ok, did a little testing and I feel like this has to be a bug in covr. If you just create a utils.R file with literally any comment in it, this also fixes the issue. I'm so confused.

Comment on lines -90 to -95

geotargets_repair_option_name <- function(option_name) {
if (!startsWith(option_name, "geotargets.")) {
option_name <- paste0("geotargets.", option_name)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be reverted

Comment on lines +1 to +5
geotargets_repair_option_name <- function(option_name) {
if (!startsWith(option_name, "geotargets.")) {
option_name <- paste0("geotargets.", option_name)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could just be:

Suggested change
geotargets_repair_option_name <- function(option_name) {
if (!startsWith(option_name, "geotargets.")) {
option_name <- paste0("geotargets.", option_name)
}
}
#howdy

And this "fix" still works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I noticed that too, it is just the presence of utils.R that makes the difference--I figured if I was gonna leave the file I should copy one of the "utility" functions into it.

@brownag
Copy link
Contributor Author

brownag commented Mar 16, 2024

Ok, did a little testing and I feel like this has to be a bug in covr.

I suspect you are right, and I think the other issue with the evaluation environment is also likely a bug

@njtierney
Copy link
Owner

Some strange {covr} bugs here! Thanks for taking the time to look into this strangeness!

@njtierney njtierney merged commit b03f527 into njtierney:master Mar 16, 2024
6 checks passed
@njtierney
Copy link
Owner

So I don't quite understand this - it was passing all checks here? Then I merged it and it broke? What in the world is happening.

@brownag
Copy link
Contributor Author

brownag commented Mar 16, 2024

The new error is a different one actually... it also is occurring on #33

the write() function in tar_format() must not create a directory. Found directories inside the data store where there should only be files: _targets/objects/test_terra_vect

This indicates that the "shz" special handling from #21 is writing a regular shapefile or something similar

From https://github.com/njtierney/geotargets/actions/runs/8305390948/job/22732222126

Run ## --------------------------------------------------------------------

R version 4.3.3 (2024-02-29) -- "Angel Food Cake"
Copyright (C) 2024 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> # This file is part of the standard setup for testthat.
> # It is recommended that you do not modify it.
> #
> # Where should you do additional test configuration?
> # Learn more about the roles of various files in:
> # * https://r-pkgs.org/testing-design.html#sec-tests-files-overview
> # * https://testthat.r-lib.org/articles/special-files.html
> 
> library(testthat)
> library(geotargets)
> 
> test_check("geotargets")
Loading required namespace: terra
▶ dispatched target test_terra_rast
● completed target test_terra_rast [0.017 seconds]
▶ ended pipeline [0.099 seconds]
Loading required namespace: terra
▶ dispatched target test_terra_vect
● completed target test_terra_vect [0.016 seconds]
▶ dispatched target test_terra_vect_shz
● completed target test_terra_vect_shz [0.04 seconds]
[ FAIL 1 | WARN 0 | SKIP 0 | PASS 2 ]

══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-tar-terra.R:42:3'): tar_terra_vect() works ─────────────────────
<tar_condition_run/tar_condition_targets/rlang_error/error/condition>
Error: Error: Error running targets::tar_make()
Error messages: targets::tar_meta(fields = error, complete_only = TRUE)
Debugging guide: https://books.ropensci.org/targets/debugging.html
How to ask for help: https://books.ropensci.org/targets/help.html
Last error message:
    the write() function in tar_format() must not create a directory. Found directories inside the data store where there should only be files: _targets/objects/test_terra_vect
Last error traceback:
    base::tryCatch(base::withCallingHandlers({ NULL base::saveRDS(base::do.c...
    tryCatchList(expr, classes, parentenv, handlers)
    tryCatchOne(tryCatchList(expr, names[-nh], parentenv, handlers[-nh]), na...
    doTryCatch(return(expr), name, parentenv, handler)
    tryCatchList(expr, names[-nh], parentenv, handlers[-nh])
    tryCatchOne(expr, names, parentenv, handlers[[1L]])
    doTryCatch(return(expr), name, parentenv, handler)
    base::withCallingHandlers({ NULL base::saveRDS(base::do.call(base::do.ca...
    base::saveRDS(base::do.call(base::do.call, base::c(base::readRDS("/tmp/R...
    base::do.call(base::do.call, base::c(base::readRDS("/tmp/Rtmpq6DjBa/call...
    (function (what, args, quote = FALSE, envir = parent.frame()) { if (!is....
    (function (targets_function, targets_arguments, options, envir = NULL, s...
    tryCatch(out <- withCallingHandlers(targets::tar_callr_inner_try(targets...
    tryCatchList(expr, classes, parentenv, handlers)
    tryCatchOne(expr, names, parentenv, handlers[[1L]])
    doTryCatch(return(expr), name, parentenv, handler)
    withCallingHandlers(targets::tar_callr_inner_try(targets_function = targ...
    targets::tar_callr_inner_try(targets_function = targets_function, target...
    do.call(targets_function, targets_arguments)
    (function (pipeline, path_store, names_quosure, shortcut, reporter, seco...
    local_init(pipeline = pipeline, meta = meta_init(path_store = path_store...
    self$end()
    tar_assert_objects_files(self$meta$store)
    tar_throw_run("the write() function in tar_format() ", "must not create ...
    tar_error(message = paste0(...), class = base::union(custom_error_classe...
    rlang::abort(message = message, class = class, call = tar_empty_envir)
    signal_abort(cnd, .file)
Backtrace:
    ▆
 1. └─targets::tar_make() at test-tar-terra.R:42:3
 2.   └─targets:::callr_outer(...)
 3.     ├─targets:::if_any(...)
 4.     └─targets:::callr_error(traced_condition = out, fun = fun)
 5.       └─targets::tar_throw_run(message, class = class(traced_condition$condition))
 6.         └─targets::tar_error(...)
 7.           └─rlang::abort(message = message, class = class, call = tar_empty_envir)

[ FAIL 1 | WARN 0 | SKIP 0 | PASS 2 ]
Error: Error: Test failures
Execution halted

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