Skip to content

Conversation

@tomjemmett
Copy link
Member

adds unit tests to the package

@matt-dray matt-dray linked an issue Feb 3, 2026 that may be closed by this pull request
@tomjemmett
Copy link
Member Author

currently hitting 80.43% coverage. I've not added tests for the fct_plots/fct_table files, largely because testing plots is tricky at best (you can try snapshots, but they will vary on OS so can be hard to build snapshots and have them run in actions reliably).

I have refactored some of the code to make it easier to test, such as adding new helper functions, or refactoring existing functions.

I have tested the app locally and functionality appears to be identical.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds comprehensive unit tests to the tpma.explorer R package, along with several code refactorings and bug fixes to improve testability and maintainability.

Changes:

  • Added comprehensive test suite covering utility functions, Shiny modules, and app components using testthat and mockery
  • Refactored fetch_strategy_text to use httr2 instead of utils::download.file for better testability
  • Fixed bug in prepare_diagnoses_data by changing from inner_join to left_join to preserve unknown diagnosis codes
  • Refactored several functions to improve separation of concerns and testability (e.g., extracting get_rates_data, get_rates_trend_data, get_peers_lookup)
  • Simplified make_strategy_group_lookup to return a named list instead of a tibble
  • Updated configuration files (.lintr, air.toml) for code quality

Reviewed changes

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

Show a summary per file
File Description
tests/testthat/*.R Comprehensive unit tests for all utility functions and Shiny modules
tests/testthat/_snaps/*.md Snapshot tests for UI components
tests/testthat.R, setup.R Standard testthat configuration
R/utils_table.R Bug fix: changed inner_join to left_join in prepare_diagnoses_data
R/utils_show_strategy_text.R Refactored to use httr2 for HTTP requests; removed convert_md_to_html
R/utils_server.R Moved get_container function from R/fct_azure.R
R/utils_plot_rates.R New utility functions extracted for better testability
R/utils_plot.R Updated generate_rates_baseline_data signature
R/utils_data.R Added md_string_to_html; refactored make_strategy_group_lookup
R/mod_*.R Various refactorings to improve testability
DESCRIPTION Added testthat and httr2 dependencies
NAMESPACE Updated exports to reflect function changes
man/*.Rd Updated documentation for refactored functions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tomjemmett and others added 10 commits February 5, 2026 09:21
@matt-dray matt-dray removed the request for review from DCEW February 5, 2026 11:11
@matt-dray matt-dray added enhancement New feature or request priority: must MoSCoW priority labels Feb 5, 2026
@matt-dray matt-dray added this to the v0.3.4 milestone Feb 5, 2026
all of the setup functions, and sample data, now exist in helper scripts.

this is useful as these helper scripts are loaded with pkgload::load_all, so you can run individual tests.

if you don't use helpers, trying to run a test in isolation can fail as it fails to source the entire test script and load in the setup functions
Copy link
Contributor

@matt-dray matt-dray left a comment

Choose a reason for hiding this comment

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

Thanks Tom, you talked me through the code changes and the tests themselves. I'm grateful for this ahead of the app's release into the wider world.

One (!) snapshot test errored for me locally: ui in test-app_ui.R. It's related to the feedback form URL, which is obfuscated as an env var. Can you adjust the ui test to use the approach I used in nhp_outputs?

@tomjemmett
Copy link
Member Author

Thanks Tom, you talked me through the code changes and the tests themselves. I'm grateful for this ahead of the app's release into the wider world.

One (!) snapshot test errored for me locally: ui in test-app_ui.R. It's related to the feedback form URL, which is obfuscated as an env var. Can you adjust the ui test to use the approach I used in nhp_outputs?

@matt-dray resolved in d3f55b7

Copy link
Contributor

@matt-dray matt-dray left a comment

Choose a reason for hiding this comment

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

Cracking, all pass.

@tomjemmett tomjemmett merged commit f579562 into main Feb 5, 2026
2 checks passed
@tomjemmett tomjemmett deleted the add_tests branch February 5, 2026 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request priority: must MoSCoW priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add basic tests for MVP

2 participants