Skip to content

Refactor r tests - #1674

Open
Bai-Li-NOAA wants to merge 6 commits into
mainfrom
refactor-r-tests
Open

Refactor r tests#1674
Bai-Li-NOAA wants to merge 6 commits into
mainfrom
refactor-r-tests

Conversation

@Bai-Li-NOAA

@Bai-Li-NOAA Bai-Li-NOAA commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

What is the feature?

  • update package data and use the data for unit tests. Now devtools::test() takes about 35 s to run locally.

How have you implemented the solution?

  • Update data-raw/data_big.R to add precomputed data (parameters, fit outputs, and estimates)
  • Update unit tests to use precomputed package data
  • Gate long-running integration tests behind RUN_SLOW_TESTS so heavy tests are optional locally and explicit in CI.
  • Split slow-test workflow by test category filter across OS matrix
  • Add workflow to keep package data updated from source changes
  • Use namespaced lookup for allowed input types in fimsframe.R by replacing fims_input_types
    with FIMS::fims_input_types(). It's for preventing "object fims_input_types not found" error when not using library(FIMS) in the data-raw/data_big.R

Does the PR impact any other area of the project, maybe another repo?


Instructions for code reviewer

👋Hello reviewer👋, thank you for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete but feel free to skip over items that are not relevant!
  • See the GitHub documentation for how to comment on a PR to indicate where you have questions or changes are needed before approving the PR.
  • Please use standard conventional messages for both commit messages and comments
  • PR reviews are a great way to learn so feel free to share your tips and tricks. However, when suggesting changes to the PR that are optional please include nit: (for nitpicking) as the comment type. For example, nit: I prefer using a data.frame() instead of a matrix because ...
  • Engage with the developer. Make it clear when the PR is approved by selecting the approved status, and potentially commenting on the PR with something like This PR is now ready to be merged.

Checklist

  • The code is well-designed
  • The code is designed well for both users and developers
  • Code coverage remains high- [ ] Comments are clear, useful, and explain why instead of what
  • Code is appropriately documented (doxygen and roxygen)

@Bai-Li-NOAA

Copy link
Copy Markdown
Collaborator Author

@kellijohnson-NOAA I need to comment out all expect_error() tests in test-rcpp-fims.R and test-rcpp-fleet-interface.R so I can run devtools::test() locally without R crashing (see issue #1363). Once issue #1363 is resolved, developers will be able to run the full test suite and benefit from the reduced test time.

@kellijohnson-NOAA

Copy link
Copy Markdown
Contributor

Can you use FIMS::fims_input_types rather than just fims_input_types to get the data types without making it a function?

@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.07%. Comparing base (9d09806) to head (e11a05a).

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1674       +/-   ##
===========================================
- Coverage   83.73%   66.07%   -17.67%     
===========================================
  Files          56       74       +18     
  Lines        2232     8108     +5876     
  Branches      532        0      -532     
===========================================
+ Hits         1869     5357     +3488     
- Misses        293     2751     +2458     
+ Partials       70        0       -70     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors FIMS’ R test suite and documentation to rely on precomputed package data objects (big dataset, parameters, fits, estimates), while gating heavyweight integration/slow tests behind RUN_SLOW_TESTS and adjusting CI workflows to run those slow categories explicitly.

Changes:

  • Add/extend precomputed package datasets (*_big) and update the data-raw/data_big.R pipeline to save fits/estimates for unit tests and vignettes.
  • Refactor many {testthat} tests to use package data objects instead of generating/reading fixture RDS files; gate heavy integration tests behind RUN_SLOW_TESTS.
  • Split the slow-tests GitHub Actions workflow by test category filter, and add a workflow to keep package data updated from source changes.

Reviewed changes

Copilot reviewed 35 out of 55 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
vignettes/fims-demo.Rmd Switch vignette examples to use precomputed fit/estimates objects instead of running long model fits.
tests/testthat/test-tidy.R Use precomputed fit/estimates and attempt to mock get_estimates() for tidy() tests.
tests/testthat/test-reshape_json_estimates.R Gate heavy integration setup behind RUN_SLOW_TESTS.
tests/testthat/test-projections-looped.R Gate heavy integration setup behind RUN_SLOW_TESTS.
tests/testthat/test-integration-proportion-female.R Gate heavy integration setup behind RUN_SLOW_TESTS.
tests/testthat/test-integration-fleet-log-obs-error-input.R Gate heavy integration setup behind RUN_SLOW_TESTS.
tests/testthat/test-integration-fims-estimation-random-effects-without-wrappers.R Gate heavy integration setup behind RUN_SLOW_TESTS.
tests/testthat/test-integration-fims-estimation-random-effects-with-wrappers.R Gate heavy integration setup behind RUN_SLOW_TESTS.
tests/testthat/test-integration-fims-bayesian-prior-predictive.R Gate heavy integration setup behind RUN_SLOW_TESTS.
tests/testthat/test-integration-caa-mle-wrappers.R Use precomputed fits/estimates for some checks; gate remaining heavy parts behind RUN_SLOW_TESTS.
tests/testthat/test-integration-caa-mle-without-wrappers.R Gate heavy integration setup behind RUN_SLOW_TESTS.
tests/testthat/test-get_version.R Refactor to validate versions using precomputed fit objects.
tests/testthat/test-get_timing.R Refactor to validate timing using precomputed fit objects.
tests/testthat/test-get_sdreport.R Refactor to validate sdreport using precomputed fit objects.
tests/testthat/test-get_opt.R Refactor to validate optimizer output using precomputed fit objects.
tests/testthat/test-get_obj.R Refactor to validate objective object using precomputed fit objects.
tests/testthat/test-get_number_parameters.R Refactor to validate parameter counts using precomputed fit objects.
tests/testthat/test-get_model_output.R Refactor to validate model output JSON using precomputed fit objects.
tests/testthat/test-get_max_gradient.R Refactor to validate gradients using precomputed fit objects.
tests/testthat/test-get_input.R Refactor to validate inputs using precomputed fit objects.
tests/testthat/test-get_estimates.R Refactor snapshots/structure checks to use precomputed estimates objects.
tests/testthat/test-fimsframe.R Switch setup to use FIMS::data_big directly.
tests/testthat/test-fimsfit.R Switch setup to use precomputed data/parameters and gate slow fit-failure tests behind RUN_SLOW_TESTS.
tests/testthat/test-create_default_configurations.R Replace fixture iteration with more targeted edge-case checks using in-memory data manipulations.
tests/testthat/helper-aaa-prompt-to-update-data.R New interactive helper to remind developers to update _big datasets when core code changes.
tests/testthat/_snaps/get_estimates.md Update snapshots to reflect precomputed estimates objects.
tests/README.md Document using precomputed package data and RUN_SLOW_TESTS gating for heavy tests.
R/fimsframe.R Use namespaced lookup for allowed input type values in validity checks.
R/data_big.R New roxygen docs for *_big datasets (data, parameters, fits, estimates).
R/data_.R Remove old data_big roxygen doc location (replaced by R/data_big.R).
pkgdown/_pkgdown.yml Broaden dataset reference index to include all *_big objects.
man/run_fims_retrospective.Rd Fix links in “Other diagnostic_functions”.
man/run_fims_likelihood.Rd Fix links in “Other diagnostic_functions”.
man/reexports.Rd Adjust reexport links for generics functions.
man/plot_likelihood.Rd Fix links in “Other diagnostic_functions”.
man/parameters_big.Rd New generated dataset documentation.
man/lognormal.Rd Adjust stats links for gaussian/family docs.
man/fit_with_optimization_big.Rd New generated dataset documentation for fit objects.
man/FIMSFrame.Rd Fix methods::showClass() link target.
man/estimates_with_optimization_big.Rd New generated dataset documentation for estimates objects.
man/data_big.Rd Update roxygen source pointer to R/data_big.R.
data-raw/data_big.R Extend data generation to also compute/save parameters, fits, and estimates as package data.
.gitignore Remove ignores for older integration plan/log directories; keep ignoring tests/testthat/fixtures/*.RDS.
.github/workflows/run-slow-tests.yml Split slow test workflow into matrix categories and run by devtools::test(filter=...).
.github/workflows/call-update-data-r.yml Add workflow hook to update /data artifacts when source/data-raw changes occur.
.devcontainer/devcontainer.json Add R packages (future, furrr, yardstick) to devcontainer feature install list.
Files not reviewed (10)
  • man/FIMSFrame.Rd: Generated file
  • man/data_big.Rd: Generated file
  • man/estimates_with_optimization_big.Rd: Generated file
  • man/fit_with_optimization_big.Rd: Generated file
  • man/lognormal.Rd: Generated file
  • man/parameters_big.Rd: Generated file
  • man/plot_likelihood.Rd: Generated file
  • man/reexports.Rd: Generated file
  • man/run_fims_likelihood.Rd: Generated file
  • man/run_fims_retrospective.Rd: Generated file
Comments suppressed due to low confidence (1)

tests/testthat/test-get_estimates.R:48

  • This test case is named as if it exercises get_estimates(), but it only checks the structure/snapshot of the precomputed estimates_with_optimization_big object. This is misleading in test reports and also reduces fast-suite coverage of get_estimates() unless other tests cover it. Consider renaming the test and adding a separate fast test that calls get_estimates() on a minimal fit object.

Comment thread tests/testthat/test-tidy.R
Comment thread tests/testthat/test-get_opt.R Outdated
Comment thread tests/testthat/test-get_timing.R Outdated
Comment thread tests/testthat/test-get_estimates.R Outdated
Comment thread tests/README.md Outdated
Comment thread .github/workflows/call-update-data-r.yml
@Bai-Li-NOAA

Copy link
Copy Markdown
Collaborator Author

@kellijohnson-NOAA

Can you use FIMS::fims_input_types rather than just fims_input_types to get the data types without making it a function?

I thought that's what I did in this commit: 7437d94. Did I turn it into a function? Copilot did suggest that I make it a function...

@kellijohnson-NOAA

Copy link
Copy Markdown
Contributor

@Bai-Li-NOAA you are right, I hadn't even looked at the code yet, just the summary in the PR comment of what was done.

* Use namespaced lookup for allowed input types
  in `fimsframe.R` by replacing `fims_input_types`
  with `FIMS::fims_input_types()`.
* Prevents "object `fims_input_types` not found"
  error when not using `library(FIMS)` in the
  `data-raw/data_big.R`
* Update `data-raw/data_big.R` to add precomputed data
  (parameters, fit outputs, and estimates)
* Update unit tests to use precomputed package data
* Gate long-running integration tests behind RUN_SLOW_TESTS so heavy
  tests are optional locally and explicit in CI.
* Split slow-test workflow by test category filter across OS matrix
* Add workflow to keep package data updated from source changes
@Bai-Li-NOAA

Copy link
Copy Markdown
Collaborator Author

I've addressed the suggestions from Copilot, and this PR is ready for human review.

The test coverage has dropped significantly after moving the integration tests out of the coverage check. I’m working on a separate branch refactor-r-tests-increase-coverage to add more unit tests. To keep this PR focused and manageable, I plan to have Copilot help generate the unit tests and then review them myself in a separate PR.

Comment thread data-raw/data_big.R
Comment on lines 408 to +409
data_big <- rbind(data_big, length_comp_data, length_age_data)
fims_frame <- FIMS::FIMSFrame(data_big)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we save data_big as a FIMSFrame object?

Comment thread data-raw/data_big.R
Comment on lines +525 to +526
fit_without_optimization_big,
estimates_without_optimization_big,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we want these as package data. I prefer to only have the optimized fit available to users.

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