pbs-assess / pacea

An R package to house Pacific Region ecosystem data to help facilitate an ecosystem approach to fisheries.
Other
14 stars 0 forks source link

Figure out how to make tests run quicker - think to do with the get_pacea_data() type functions. #30

Closed andrew-edwards closed 10 months ago

andrew-edwards commented 11 months ago

Not sure the best way we can do this - either create some dummy data within the package (maybe a bit messy with all the data objects), or just get it to ignore some of the tests when pushing to GitHub or building.

Definitely want to keep the tests around for actual manual testing now and again, but once a function is tested maybe get it ignored when running test() or pushing to GitHub (I think there's a way to do this, and can help out).

seananderson commented 11 months ago

Assuming testthat,

skip_on_ci()

is your main one here. Other useful ones:

skip_on_cran()
skip_if_not_installed("packageNameHere")
andrew-edwards commented 11 months ago

Thanks Sean - I think I spotted you had some skip_on_ci() somewhere in a repo. We will try that.

andrew-edwards commented 11 months ago

@travistai2 - so locally one of your tests fails for me, but it passes for GHA:

> covr::package_coverage(type = "tests")
Error: Failure in `C:/Users/edwardsand/AppData/Local/Temp/RtmpeWL1ML/R_LIBS339031af5b70/pacea/pacea-tests/testthat.Rout.fail`
L 1 | WARN 1 | SKIP 0 | PASS 95 ]

══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-shortcuts-get-data.R:20:3'): shortcuts work ────────────────────
Error in `readRDS(local_file_dir)`: error reading from connection
Backtrace:
    ▆
 1. ├─testthat::expect_length(...) at test-shortcuts-get-data.R:20:2
 2. │ └─testthat::quasi_label(enquo(object), arg = "object")
 3. │   └─rlang::eval_bare(expr, quo_get_env(quo))
 4. └─pacea::roms_avg0to40m_temperature(force = TRUE)
 5.   └─pacea::get_pacea_data(...)
 6.     └─base::readRDS(local_file_dir)

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

Looks like this one (the second one passes):

> expect_length(roms_avg0to40m_temperature(force = TRUE), 325)
Error in readRDS(local_file_dir) : error reading from connection
In addition: Warning message:
In readRDS(local_file_dir) : lzma decoding result 10
> expect_length(roms_avg0to40m_salinity(force = TRUE), 325)
> 

Maybe I just need to update pacea-data or something (whereas GHA installs the latest). Just means I can't test coverage locally for now, I'll add Sean's suggestion in to see if it speeds up the GHA testing.

andrew-edwards commented 11 months ago

That commit above indeed only took a few minutes to test and check coverage on GHA. We should figure out the optimal solution, or maybe just have to do manual checks when changing the relevant functions.

travistai2 commented 11 months ago

Before I fetched line you added skip_on_ci(), I ran the test again and it passed fine. I wouldn't say the test was fast, but definitely not slow; it was about 108 seconds.

If you have all the data downloaded to your cache folder it should be fine. Running the test the first time will download all the data to your cache, which will take a while. But it should be reasonable after that.

One issue I ran into was stopping the test once when it was taking too long. This ended up downloading only half of one of the files. What ends up happening is the function sees that you have the data already in your cache so it doesn't attempt to download it from the internet (pacea-data). And because the data is only half downloaded, an error arises as R can't load it for the testthat function. So this may be the error you're getting?

travistai2 commented 11 months ago

Having said that, I'll need to write something into the function that is able to detect an incomplete file (where someone may have stopped the downloading) and re-download the file if it's incomplete.

andrew-edwards commented 11 months ago

Aha - yes, the incomplete data probably explains the error I had. Though on GitHub it always starts with a fresh build, so will have to download ROMS data, which is the time consuming bit. But it sounds like it will work locally for me once I've done all the data caching (then I can just do test() to run all tests and that will take a couple of minutes).