Convert tests from testthat to testit#625
Conversation
Replace the testthat test infrastructure with testit: - test_that() -> assert() - expect_equal() -> isTRUE(all.equal()) - expect_identical() -> %==% - expect_true/false() -> (expr) / (!(expr)) - expect_error/warning() -> has_error/has_warning() - Snapshot tests (as_gt, as_rtf) converted to testit's .md format - Test runner: tests/test-all.R using test_pkg() - DESCRIPTION: testthat dependency replaced with testit Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Upstream added as.data.frame() wrapping in test comparisons; resolved by applying the same change in testit syntax. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use the new `message` parameter in has_error() to verify specific error messages, matching the original testthat tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Since testit's assert() checks whether the value is TRUE, all.equal() already returns TRUE on success, making isTRUE() redundant. Replace (isTRUE(all.equal(a, b))) with (all.equal(a, b)). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ion 2 testthat edition 2's expect_equal() passes if EITHER the relative OR absolute difference is within tolerance. Add an all_equal() helper that replicates this behavior, and use it across all test files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@jdblischak @LittleBeannie This PR is ready for review. |
For tests comparing exact values (integers, rounded data frames, NULL, lists), use testit's %==% operator instead of all_equal(). This gives better failure diagnostics via str() output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jdblischak
left a comment
There was a problem hiding this comment.
I confirmed that the test coverage reported locally by covr::package_coverage() before and after is identical (93.12%) 🎉
A few ergonomic questions:
- Why does {testit} need to be loaded in order to run
test_pkg()? I couldn't find this in the docs
packageVersion("testit")
## [1] ‘0.18.5’
testit::test_pkg()
## Error from sys.source2(r, envir = env, top.env = ns)
## Error in assert("unstratified population, compared with old version", :
## could not find function "assert"- Is it possible to suppress the many error messages printed to the R console? I assume these are from tests that use
has_error()
library("testit")
test_pkg()
# Lots of error messages printed to the console, eg
## Error: missing value where TRUE/FALSE needed
## Error: missing value where TRUE/FALSE needed
## Error: `times` (c(1, 2, 1)) must be positive and strictly increasing!
## Error: `survival` (c("0.5", "NA")) must be positive!
## Error: `survival` must be of same length as `times`
## Error: `survival` (c(0.5, -0.1)) must be positive!
## Error: `survival` must be non-increasing
## Error: `survival` must be non-increasing
# But they all passed
.Last.value
## NULL-
I observed that {testit} stops on the first error. Is there any plan to enable {testit} to collect test errors to spot patterns, or is this out of scope?
-
For quick feedback, I often use the argument
filterofdevtools::test(), egdevtools::test(filter = "npe"). Is it possible to do something similar with {testit}?
- Tighten some all.equal() tests to use %==% for exact equality - Extract lengthy LHS/RHS expressions of %==% and all.equal() into named variables (res, expected) for clarity - Raise () assertions from inside loops to top-level using vapply() - Break excessively long lines (>120 chars) into multiple lines - Add parentheses for operator precedence with %==% (e.g., 3L * 5L) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
53a54f1 to
4d9628c
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
8f9ffd3 to
98e7a7c
Compare
|
@jdblischak All ergonomic issues have been addressed in testit. May thanks to all your great suggestion! I think they improved the usability of this package by 10 times. |
The test "s2pwe fails to identify infinity value" used `times2` (c(1, NA) from a previous test block) instead of `times3` (c(1, Inf)). It appeared to pass because `s2pwe(times = c(1, NA), ...)` does error, but the intent was to test Inf handling. With the correct variable, s2pwe(times = c(1, Inf), ...) returns a valid result (Inf is numeric and positive), so the test is invalid — there is no Inf validation in s2pwe. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Faithfully translate expect_error(expr, "message") from the original testthat tests to has_error(expr, "message") in testit, preserving the original message strings where the function still produces them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jdblischak
left a comment
There was a problem hiding this comment.
Given the recent updates to {testit} (thanks @yihui!), I am supportive of this migration of the testing framework.
I observed a slight reduction in the code coverage locally via covr::package_coverage() (93.12% to 92.91%), but according to Codev the coverage is unchanged at 92.90%.
I would like to delay merging until 1) we switch to tagged versions of yihui/actions, and 2) the updated version of {testit} is uploaded to CRAN.
218e0be to
9022c8a
Compare
9022c8a to
eb40106
Compare
|
@jdblischak Actions have been tagged. The CRAN release of testit can be made at any time if you don't have further suggestions or requests. |
yihui
left a comment
There was a problem hiding this comment.
This PR is ready for review. I'll release testit v1.0 to CRAN later today and switch to that version accordingly once it lands on CRAN.
To help reviewers understand the changes better, I need to clarify one extra thing I did in this PR, which I should have saved for another PR but I was not sure if it'd be worth torturing you one more time :) The extra thing was that I tightened the equality tests (i.e. expect_equal()) as much as possible for greater rigor. The logic is the following:
- Switch to
identical()(or equivalently,%==%from testit) whenever possible. If the two objects are strictly identical to each other, we use%==%instead of testing for approximate equality. - When we test things like
nrow()orncol()that are integers, we also try identical testing by changing the target to an integer, e.g.,expect_equal(nrow(z1$`_footnotes`), 1)is changed to(nrow(z1$`_footnotes`) %==% 1L)(note the change from double1to integer1L); this is because1is not identical to1Lalthough they are "equal". - Then for the rest of equality tests, try
all.equal()with default tolerance (i.e.sqrt(.Machine$double.eps)). If that works, we just use the defaultall.equal(). - If the default tolerance is too tight, we raise it to the nearly smallest tolerance possible to make the test pass, e.g., if the actual difference is 0.00085, we use 0.001. In our original tests, tolerances (if explicitly provided) are often too high, e.g., when 0.003 works but we used 0.01.
- When comparing probabilities, we usually use the argument
scale = 1for testing the absolute difference between two probabilities. This gives us a crystal clear idea about how much the two probabilities differ.
BTW, if test coverage is a concern, I think we can easily address it in the next PR.
I'll also add a skill to give AI models instructions on how to write tests with testit in future.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jdblischak
left a comment
There was a problem hiding this comment.
The improvements to the tolerances are very welcome! These have long been a source of frustration for me.
Let's merge once the latest {testit} is available from CRAN.
…he tolerance for `ubuntu-latest (release)` to pass
|
Just noticed this NOTE. Could you please add |
|
Sure. Done. |
|
Reminder to please squash and merge this PR |
|
I'm not sure who are admins of this repo, but they can go to https://github.com/Merck/gsDesign2/settings and disable "merge commits" and "rebase merging", which is what I do for all my repositories since I rarely need the full commit history of a PR and I always squash and merge:
|
There was a problem hiding this comment.
Pull request overview
This pull request migrates the package’s test suite from testthat to testit, aiming to reduce dependency weight while keeping equivalent coverage for design/power utilities and snapshot-style outputs.
Changes:
- Replaced
testthat::test_that()/expect_*()usage withtestit::assert()and testit-style comparisons (%==%,all.equal(),has_error()) across the test suite. - Moved snapshot-style checks to testit’s markdown snapshot format and removed legacy
testthatsnapshot artifacts. - Updated package metadata and CI workflows to reflect the new testing stack.
Reviewed changes
Copilot reviewed 102 out of 125 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| DESCRIPTION | Replaces testthat with testit in Suggests. |
| tests/test-all.R | New test runner entrypoint for testit (test_pkg("gsDesign2")). |
| tests/testthat.R | Removed legacy testthat runner. |
| tests/testthat/test-independent-gs_design_combo.R | Removed testthat version of these tests. |
| tests/testthat/test-independent-expected_time.R | Removed testthat version of these tests. |
| tests/testthat/test-independent-expected_event.R | Removed testthat version of these tests. |
| tests/testthat/test-independent-expected_accrual.R | Removed testthat version of these tests. |
| tests/testthat/test-independent-check_arg.R | Removed testthat version of these tests. |
| tests/testthat/test-independent_s2pwe.R | Removed testthat version of these tests. |
| tests/testthat/test-independent_ppwe.R | Removed testthat version of these tests. |
| tests/testthat/test-independent_as_rtf.R | Removed testthat snapshot tests for RTF output. |
| tests/testthat/test-independent_as_gt.R | Removed testthat snapshot tests for gt output. |
| tests/testthat/test-developer-gs_spending_bound.R | Removed testthat developer test. |
| tests/testthat/test-developer-gs_b.R | Removed testthat developer test. |
| tests/testthat/_snaps/independent_as_rtf/gs_design_wlr.rtf | Removed testthat snapshot artifact. |
| tests/testthat/_snaps/independent_as_rtf/gs_design_rd.rtf | Removed testthat snapshot artifact. |
| tests/testthat/_snaps/independent_as_rtf/gs_design_ahr.rtf | Removed testthat snapshot artifact. |
| tests/testthat/_snaps/independent_as_rtf/fixed_design_ahr.rtf | Removed testthat snapshot artifact. |
| tests/testthat/_snaps/independent_as_rtf/fixed_design_ahr_title.rtf | Removed testthat snapshot artifact. |
| tests/testthat/_snaps/independent_as_rtf/fixed_design_ahr_footnote.rtf | Removed testthat snapshot artifact. |
| tests/testit/helper.R | Adds shared helper (gt_to_latex) for testit tests. |
| tests/testit/helper-support-as_rtf.R | Adds helpers used by testit snapshot-style RTF tests. |
| tests/testit/helper-double-programming-expected_accrual.R | Adds double-programming helper for accrual validation. |
| tests/testit/helper-double-programming-expected_event.R | Adds double-programming helper for event validation. |
| tests/testit/helper-double-programming-expected_time.R | Adds helper for expected_time validation. |
| tests/testit/helper-double-programming-gs_design_combo.R | Adds helper for gs_design_combo validation. |
| tests/testit/helper-double-programming-gs_info_ahr.R | Adds helper for gs_info_ahr validation. |
| tests/testit/helper-double-programming-ppwe.R | Adds double-programming helpers for ppwe validation. |
| tests/testit/helper-old-version-tEvents_.R | Adds old-version helper code used for regression-style comparisons. |
| tests/testit/helper-old-version-gs_power_ahr_.R | Adds old-version helper code used for regression-style comparisons. |
| tests/testit/helper-old-version-gs_info_ahr_.R | Adds old-version helper code used for regression-style comparisons. |
| tests/testit/helper-old-version-eEvents_.R | Adds old-version helper code used for regression-style comparisons. |
| tests/testit/helper-old-version-eAccrual_.R | Adds old-version helper code used for regression-style comparisons. |
| tests/testit/test-independent-wlr_weight.R | Converts tests to testit for WLR weight utilities. |
| tests/testit/test-independent-utility_tbl.R | Converts tbl utility tests to testit. |
| tests/testit/test-independent-to_integer.R | Converts to_integer tests to testit. |
| tests/testit/test-independent-rmst.R | Converts RMST tests to testit. |
| tests/testit/test-independent-hupdate.R | Converts hupdate tests to testit. |
| tests/testit/test-independent-h1.R | Converts h1 tests to testit. |
| tests/testit/test-independent-gs_update_ahr.R | Converts gs_update_ahr tests to testit. |
| tests/testit/test-independent-gs_spending_combo.R | Converts gs_spending_combo tests to testit. |
| tests/testit/test-independent-gs_spending_bound.R | Converts gs_spending_bound tests to testit. |
| tests/testit/test-independent-gs_power_npe.R | Converts gs_power_npe tests to testit. |
| tests/testit/test-independent-gs_power_combo.R | Converts gs_power_combo tests to testit. |
| tests/testit/test-independent-gs_power_ahr.R | Converts gs_power_ahr tests to testit. |
| tests/testit/test-independent-gs_info_combo.R | Converts gs_info_combo tests to testit. |
| tests/testit/test-independent-gs_info_ahr.R | Converts gs_info_ahr tests to testit. |
| tests/testit/test-independent-gs_design_combo.R | Adds testit version of gs_design_combo tests. |
| tests/testit/test-independent-gs_design_ahr.R | Converts gs_design_ahr tests to testit. |
| tests/testit/test-independent-gs_bound.R | Converts gs_bound tests to testit. |
| tests/testit/test-independent-gs_b.R | Converts gs_b tests to testit. |
| tests/testit/test-independent-gridpts.R | Converts gridpts tests to testit. |
| tests/testit/test-independent-fixed_design.R | Converts fixed design tests to testit. |
| tests/testit/test-independent-expected_time.R | Adds testit version of expected_time tests. |
| tests/testit/test-independent-expected_event.R | Adds testit version of expected_event tests. |
| tests/testit/test-independent-expected_accrual.R | Adds testit version of expected_accrual tests. |
| tests/testit/test-independent-check_arg.R | Adds testit version of argument-check tests. |
| tests/testit/test-independent_s2pwe.R | Adds testit version of s2pwe tests. |
| tests/testit/test-independent_ppwe.R | Adds testit version of ppwe tests. |
| tests/testit/test-independent_as_gt.R | Adds basic non-snapshot as_gt-related tests under testit. |
| tests/testit/test-independent-ahr.R | Converts ahr tests to testit. |
| tests/testit/test-independent-ahr_blinded.R | Converts ahr_blinded tests to testit. |
| tests/testit/test-independent_gs_power_wlr.R | Converts gs_power_wlr tests to testit. |
| tests/testit/test-independent_gs_info_wlr.R | Converts gs_info_wlr tests to testit. |
| tests/testit/test-independent_gs_design_wlr.R | Converts gs_design_wlr tests to testit. |
| tests/testit/test-developer-sequential_pval.R | Converts sequential_pval developer tests to testit. |
| tests/testit/test-developer-pw_info.R | Converts pw_info developer tests to testit. |
| tests/testit/test-developer-gs_spending_bound.R | Adds testit version of gs_spending_bound developer test. |
| tests/testit/test-developer-gs_power_wlr.R | Converts gs_power_wlr developer tests to testit. |
| tests/testit/test-developer-gs_power_rd.R | Converts gs_power_rd developer tests to testit. |
| tests/testit/test-developer-gs_power_npe.R | Converts gs_power_npe developer tests to testit. |
| tests/testit/test-developer-gs_info_wlr.R | Converts gs_info_wlr developer tests to testit. |
| tests/testit/test-developer-gs_info_rd.R | Converts gs_info_rd developer tests to testit. |
| tests/testit/test-developer-gs_design_rd.R | Converts gs_design_rd developer tests to testit. |
| tests/testit/test-developer-gs_design_npe.R | Converts gs_design_npe developer tests to testit. |
| tests/testit/test-developer-gs_cp.R | Converts gs_cp developer tests to testit. |
| tests/testit/test-developer-gs_cp_simple.R | Converts gs_cp_simple developer tests to testit. |
| tests/testit/test-developer-gs_cp_npe2.R | Converts gs_cp_npe2 developer tests to testit. |
| tests/testit/test-developer-gs_cp_npe1.R | Converts gs_cp_npe1 developer tests to testit. |
| tests/testit/test-developer-gs_b.R | Adds testit version of gs_b developer test. |
| tests/testit/test-developer-gridpts.R | Converts gridpts developer tests to testit. |
| tests/testit/test-developer-fastlag.R | Converts fastlag developer tests to testit. |
| tests/testit/test-developer-expected_time.R | Converts expected_time developer tests to testit. |
| tests/testit/test-developer-expected_event.R | Converts expected_event developer tests to testit. |
| tests/testit/test-developer-as_gt.R | Converts as_gt developer tests to testit. |
| tests/testit/test-developer-ahr.R | Converts ahr developer tests to testit. |
| tests/testit/fixtures/simulation_test_data.Rdata | Adds/updates fixture data used by tests. |
| tests/testit/fixtures/sim_gsd_pMaxCombo_exp1_H1_test.Rdata | Adds/updates fixture data used by tests. |
| tests/testit/fixtures/sim_gsd_pMaxCombo_exp1_H0_test.Rdata | Adds/updates fixture data used by tests. |
| .github/workflows/test-coverage.yaml | Updates coverage workflow dependency setup consistent with testit migration. |
| .github/workflows/R-CMD-check.yaml | Updates R CMD check workflow actions/dependency setup. |
| .Rbuildignore | Ignores .claude artifacts. |
| .gitignore | Minor formatting cleanup. |
| gsDesign2.Rproj | Removes PackageUseDevtools setting. |
| .claude/skills/write-tests.md | Adds repository guidance for writing testit-based tests. |
Comments suppressed due to low confidence (4)
tests/testit/test-independent_gs_power_wlr.R:100
- This compares the Z boundaries with
%==%(exactidentical()), but these are floating-point values coming from numerical routines and were previously checked with a tolerance. Using exact comparison here is likely to make the test flaky across platforms/R versions; consider switching back toall.equal(..., tolerance = ...)(e.g., the prior 0.1 tolerance).
tests/testit/test-independent-ahr_blinded.R:27 - These assertions use
%==%(exactidentical()) forahr,theta, andinfo0, but the expected values are computed via floating-point ops and the old test used tolerances. To reduce cross-platform flakiness, useall.equal(..., tolerance = ...)for these numeric comparisons (and keep%==%for the integer event count if desired).
tests/testit/test-developer-gs_info_rd.R:60 resandexpectedhere are computed from floating-point arithmetic, so comparing with%==%(exactidentical()) is brittle and may fail due to minor numerical differences. Useall.equal(res, expected, tolerance = ...)(similar to the surrounding assertions) for these checks.
This issue also appears on line 84 of the same file.
tests/testit/test-developer-gs_info_rd.R:90
- Same issue as above: this compares a floating-point result with
%==%(exactidentical()), which is likely to be flaky. Preferall.equal(res, expected, tolerance = ...)for numeric equality here.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Summary
as_gt,as_rtf) converted to testit's markdown-based.mdformat with output embedded inlinetestthat (>= 3.0.0)replaced withtestittest_that()→assert(),expect_equal()→all.equal(),expect_identical()→%==%,expect_error()→has_error(),expect_true()→(expr)Motivation
More info: https://yihui.org/en/2026/05/testthat-to-testit/