testthat unit tests are failing when using mocks with httptest2.

[mccroweyclinton-EPA]

I have noticed that when unit tests are ran using the RStudio Test button in the Environment/History/Connections/Build/Git/Tutorial pane sometimes unit test work, but when running unit test using testthat::test_local() most of the time unit test fail. The results are not predictable as far as I can tell. Sometimes it works, and other times the test don't work.

@mpadge has suggested that the issue is probably related to testthat2's redaction facilities (inst/httptest2/redact.R). I am in agreement with @mpadge's suggestion, in particular, I believe that this issue is related to the two calls to

getOption()

on lines 10 and 11. My initial thoughts are that for some reason, in some contexts, these two function calls are not returning the expected results.

Here is the output from a failed run:

Running the tests in 'tests/testthat.R' failed.
Last 13 lines of output:
2. └─testthat::expect_no_error(.)
── Failure ('test-bystate.R:150:5'): bystate functions ─────────────────────────
Expected . to run without any errors.
i Actually got a <purrr_error_indexed> with text:
i In index: 1.
Caused by error in gsub():
! zero-length pattern
Backtrace:
▆
1. ├─... %>% expect_no_error() at test-bystate.R:150:5
2. └─testthat::expect_no_error(.)

 [ FAIL 73 | WARN 0 | SKIP 0 | PASS 26 ]
 Error: Test failures
 Execution halted

[mpadge]

@mccroweyclinton-EPA Seems that your test coverage workflows have not yet passed at all. They provide a solid benchmark, as they are run in entirely independent machines which you must explicitly configure to get to succeed. They also provide both detailed output on screen, like this:

and if needed, you can also download the complete logs by looking at the end of the Upload test results step:

---

In your case, the first bit is pretty clear:

path too long

Those problems are easily solved by httptest2 redaction. An example of redacting to reduce path lengths is https://github.com/ropensci-review-tools/pkgcheck/blob/main/inst/httptest2/redact.R:

    resp <- httptest2::gsub_response (
        resp,
        "ropensci-review-tools/pkgcheck/git/trees/",
        "",
        fixed = TRUE
    )

For URLs or local file paths, httptest2 creates sub-directories for each path/URL separator. This can easily create final paths that are too long for base R operations. That example entirely removes the four sub-directories ropensci-review-tools/pkgcheck/git/trees/ by replacing with an empty "". You at least need to start with something along those lines. In particular, you have mock test sub-directories with extremely long names like "transactionsQaAnnualPerformanceEvaluations" which should definitely be redacted to something much shorter. I think the base R path limit is 80 characters (or maybe 120?), and that alone is 42 characters. You also have dirs which only have sub-dirs (.../datamart), which should also be redacted down to single dirs.

In short: You first need to get your test-coverage action working on GitHub.

[mccroweyclinton-EPA]

@mpadge, That is a different error than what I am currently getting.

I am sorry, I fixed that issue a few weeks ago. I guess I never got around to pushing my latest changes. I just created a new branch, test_httptest2 with my latest code changes. Please look at this branch.

[mpadge]

To reiterate:

You first need to get your test-coverage action working on GitHub.

It's not fixed if that's not working. You'll need to first get it working here, then ensure pkgcheck gives the all clear.

[mccroweyclinton-EPA]

To reiterate:

You first need to get your test-coverage action working on GitHub.

It's not fixed if that's not working. You'll need to first get it working here, then ensure pkgcheck gives the all clear.

@mpadge, I don't think you understand. I am not worried about the github actions stuff, ATM. These failures are happening when I run testthat locally. If the unit test are failing while being run locally, then I know that there is an issue somewhere in the code that has nothing to do with how the github actions is setup.

[mccroweyclinton-EPA]

The problem has to be somehow related to the inst/httptest2/redact.R file. Because when I remove it, clear the cached mocks then rerun test that on my local machine, the unit test run without a problem. Reruning unit test locally with mocks seem to work fine as well.

There may be another separate issue with the test-coverage workflow but that is a different issue. For now I am focused on getting unit test with mocks and httptest2 redacting capabilities to just run in a local testing environment. I will worry about the github workflows after I can fix this issue.

If the unit test are failing locally then there is no point in trying to make them work on github's remote servers.

[mccroweyclinton-EPA]

@gaborcsardi and @mpadge, I don't know why but it is running fine both locally and on the R-CMD-check workflow now. I'll leave this issue open for a while to see if it cause a problem again. The unit test seem to work sometimes and at other times seem to fail. I have not figured out why they seem to fail at random times.

[mccroweyclinton-EPA]

I just ran testthat::test_local again and this time I am getting the errors above. I can not figure out why unit test run sometimes, and other times it does not.

[mpadge]

@mccroweyclinton-EPA Intermittent failures almost always reflect some local quirks of your own setup. A first step should be to try to setup a more repeatable and robust environment. My general approach for that is to start a clean docker container (like rocker/tidyverse), and run tests there to debug. Or, as I said above, the GitHub workflows also provide very useful benchmarks, as they are always run in clean and entirely reproducible environments. The outputs I linked to above clearly showed issues that need fixing regardless of any others. Working through those, and iterating either remotely on GitHub or locally on docker (or whatever), fixing each failure identified, will likely be the best way for you to resolve issues.

[maelle]

👋 @mccroweyclinton-EPA! @ldecicco-USGS mentioned your package to me (because I wrote https://books.ropensci.org/http-testing/httptest2.html -- but these things can always be tricky!). I have some comments/questions

• What's the use case for RAQSAPItestsetup_helper(), especially the path? Could https://testthat.r-lib.org/reference/test_path.html cover the logic related to finding local.R but what is local.R? It's a file that only contributors have on their machine? Why not store the credentials with keyring instead? And then still having the fallback for when you don't have credentials. So in tests/testthat/helper.R you'd have something like:

api_key <- try(keyring::key_get("bla"), silent = TRUE)
if (inherits(api_key, "try-error") {
  api_key <- "fake-api-key"
}
# same for username

For creating the mock files one would need to have real credentials but not for running tests once mock files have been created.

• I don't think aqs_credentials() should use options. Functions that use the options should instead of using getOption(), call aqs_credentials() that would return the credentials as a list like what RAQSAPItestsetup_helper() currently does.
• In tests, you do not need to source helper files, they are loading automatically by testthat. https://r-pkgs.org/testing-design.html#sec-tests-files-overview
• In tests, because your package is also loaded by testthat, there's no need to namespace calls to functions of the package, so you can remove all RAQSAPI:: (that is also true of functions internal to your package, not only exported functions).
• In tests you also don't need to load testthat through roxygen2 comments:

#' @importFrom magrittr `%>%`()
#' @import testthat
#' @import httptest2

• The test errors I am getting are mostly related to httptest2 not finding the mock files. What I'd do in a package of mine is deleting all mock files then re-running the tests again to see whether they are created and where exactly.
• Regarding the redact.R file: does this mean the API includes the user's API key and username in the response by default?!
• What's the role of those lines at the top of redact.R?

if (getOption("raqsapi.redactor_installed", FALSE)) {
  invisible(NULL)
} else {
  options(raqsapi.redactor_installed = TRUE)
}

• Your tests use expect_no_error() a lot, which is not very specific. It might be better to assign the result of the function call, say aqs_qa_collocated_assessments_by_MA() to an object then ensuring this object has the correct class, column names, etc.
• I'd recommend starting from main again, creating a new branch, then setting one single test up with httptest2 and build up from there.

Please tell me whether you have any question. As I mentioned earlier, this stuff can be hard to get right the first time (or the times after that 😹 )!

[mccroweyclinton-EPA]

👋 @mccroweyclinton-EPA! @ldecicco-USGS mentioned your package to me (because I wrote https://books.ropensci.org/http-testing/httptest2.html -- but these things can always be tricky!). I have some comments/questions

• Thank you for your help. I am glad to work with you

What's the use case for RAQSAPItestsetup_helper(), especially the path? Could https://testthat.r-lib.org/reference/test_path.html cover the logic related to finding local.R but what is local.R? It's a file that only contributors have on their machine? Why not store the credentials with keyring instead? And then still having the fallback for when you don't have credentials. So in tests/testthat/helper.R you'd have something like:

• pretty much what you've just said, it is a file that only I have that basically calls keyring and retrieves the credentials and returns those credentials to the unit test. I could put that in the file but it contains that user that keyring uses to access the credentials. Not a big deal but I don't want to expose the username being used for testing so I put it in a helper file.

I don't think aqs_credentials() should use options. Functions that use the options should instead of using getOption(), call aqs_credentials() that would return the credentials as a list like what RAQSAPItestsetup_helper() currently does.

• aqs_credentials() uses options to store user credentials because user credentials are not hard coded into the code, credentials are stored as a option so that credentials can be retrieved once aqs_credentials() is out of scope.

In tests, you do not need to source helper files, they are loading automatically by testthat. https://r-pkgs.org/testing-design.html#sec-tests-files-overview

• Yes, I saw that but for some reason at one point helper.R was not being loaded into memory, so I just sourced it. Maybe that wa sdue to an old bug that has since been fixed, I can remove that code from unittests

In tests you also don't need to load testthat through roxygen2 comments:

The same issue happend here before, I just loaded testthat because I was having issues before, maybe that issue has since been fixed, I can remove this.

In tests, because your package is also loaded by testthat, there's no need to namespace calls to functions of the package, so you can remove all RAQSAPI:: (that is also true of functions internal to your package, not only exported functions).

• I am aware of this, didn't think it would be a big deal but I can remove the namespace calls to RAQSAPI.

The test errors I am getting are mostly related to httptest2 not finding the mock files. What I'd do in a package of mine is deleting all mock files then re-running the tests again to see whether they are created and where exactly.

• I have tried this many times, as you can see the mock files have been generated and have been committed in the test_httptest2 branch under the test/testthat/ directory but httptest2 is still not finding those mocks.

Regarding the redact.R file: does this mean the API includes the user's API key and username in the response by default?!

• yes the credentials are included in the url that is generated, the url that was used to make the request is returned in the headers url field.

What's the role of those lines at the top of redact.R?

if (getOption("raqsapi.redactor_installed", FALSE)) {
  invisible(NULL)
} else {
  options(raqsapi.redactor_installed = TRUE)
}

Those lines can probably be removed