Improved composition #2250

hadley · 2025-10-03T19:31:07Z

expect_named() and expect_output() need to always return the input value, even if they use some subexpectation.

To make this work, expectation components now only ever fail (never pass) and return TRUE or FALSE.

Fixes expect_named() and expect_output() now return different outputs

`expect_named()` and `expect_output()` need to always return the input value, even if they use some subexpectation. To make this work, expectation components now only ever fail (never pass) and return TRUE or FALSE. Fixes `expect_named()` and `expect_output()` now return different outputs Fixes #2246

hadley · 2025-10-03T22:18:37Z

@DavisVaughan can you please take a fairly deep look at this? I think it's probably easier to look at the flow of the new versions, rather than inspecting the diffs. Assuming that this looks good to you, I'll merge this PR and then rewrite all the expectations to follow this new pattern.

I do think separating the return value (which is now always invisible(act$val)) from success/failure is a substantial improvement. Previously the two were tangled together in a rather confusing way. Changing the return value of failures shouldn't have an implications on revdeps since it only matters if they fail, which obviously can't happen on CRAN.

It also leads to a less confusing failure when you have a sequence of expectations.

library(testthat)

test_that("", {
  1:10 |> 
    expect_type("character") |> 
    expect_length(10)
})

# BEFORE
#> ── Failure:  ────────────
#> Expected `1:10` to have type "character".
#> Actual type: "integer"
#> ── Failure:  ────────────
#> Expected `expect_type(1:10, "character")` to have length 10.
#> Actual length: 3.
#> Error:
#> ! Test failed with 2 failures and 0 successes.

# AFTER
#> ── Failure:  ────────────
#> Expected `1:10` to have type "character".
#> Actual type: "integer"
#> Error:
#> ! Test failed with 1 failure and 1 success.

hadley · 2025-10-05T22:44:11Z

I updated a few more — early returns now feel pretty suboptimal, so I only used them when it really saves a bunch of indenting. (But even there it might be better to create a helper function)

hadley · 2025-10-06T13:09:38Z

I've convinced myself that this is a good idea so I'm converting all expectations. @DavisVaughan it would still be super useful for you to take a look at few expectations and critique the style.

DavisVaughan

I think this all looks great, with the main outcomes being

We learned pass() and fail() should be used purely for their side effects. In practice, this makes their usages very symmetric looking, which is very appealing to read.
We learned (nearly) all expectations should return invisible(act$val). This makes the return value orthogonal from pass() and fail() which feels very correct to me.

My main comments in the code would be:

I like seeing pass() in the if branch and fail() in the else branch (unless it is a series of fail, fail, fail, pass) and there are a lot of places we don't do that. Up to you.
Consider what side effecty helpers like expect_compare_() should return. I'd argue nothing - they are extensions of pass() and fail() and should be used purely for their side effects. Could even be renamed pass_fail_compare() to make this clear. Main argument against this was expect_message() with expect_condition_matching_() but I do not think you should let that 1 case stop you

DavisVaughan · 2025-10-06T14:02:21Z

NEWS.md

@@ -1,5 +1,6 @@
 # testthat (development version)

+* Expectations now consistently return the value of the first argument, regardless of whether the expectation succeeds or fails. The primary exception are `expect_message()` and friends which will return the condition. This shouldn't affect existing tests, but will make failures clearer when you chain together multiple expectations (#2246).


Suggested change

* Expectations now consistently return the value of the first argument, regardless of whether the expectation succeeds or fails. The primary exception are `expect_message()` and friends which will return the condition. This shouldn't affect existing tests, but will make failures clearer when you chain together multiple expectations (#2246).

* Expectations now consistently return the value of the first argument, regardless of whether the expectation succeeds or fails. The primary exceptions are `expect_message()` and friends which will return the condition. This shouldn't affect existing tests, but will make failures clearer when you chain together multiple expectations (#2246).

DavisVaughan · 2025-10-06T14:20:50Z

R/expect-comparison.R

      call = trace_env
    )
  }
-  if (!isTRUE(cmp)) {


I'm totally with you on no longer wanting to do an early return anymore

Like, I think seeing an if/else construct where one branch ends in a pass() and one branch ends in a fail() could be a defining feature of all of these expectations.

In other words, I prefer this version of expect_compare_() much more

expect_compare_ <- function( operator = c("<", "<=", ">", ">="), act, exp, trace_env = caller_env() ) { operator <- match.arg(operator) op <- match.fun(operator) actual_op <- switch(operator, "<" = ">=", "<=" = ">", ">" = "<=", ">=" = "<") cmp <- op(act$val, exp$val) if (length(cmp) != 1 || !is.logical(cmp)) { cli::cli_abort( "Result of comparison must be a single logical value.", call = trace_env ) } if (isTRUE(cmp)) { pass() } else { diff <- act$val - exp$val msg_exp <- sprintf("Expected %s %s %s.", act$lab, operator, exp$lab) digits <- max( digits(act$val), digits(exp$val), min_digits(act$val, exp$val) ) msg_act <- sprintf( "Actual comparison: %s %s %s", num_exact(act$val, digits), actual_op, num_exact(exp$val, digits) ) if (is.na(diff)) { msg_diff <- NULL } else { msg_diff <- sprintf( "Difference: %s %s 0", num_exact(diff, digits), actual_op ) } fail(c(msg_exp, msg_act, msg_diff), trace_env = trace_env) } invisible(act$val) }

Yeah, I was thinking that instead of an early return, pulling out a helper to generate the failure message would be the way to go.

yea everything except the fail(), i.e. this bit

diff <- act$val - exp$val msg_exp <- sprintf("Expected %s %s %s.", act$lab, operator, exp$lab) digits <- max( digits(act$val), digits(exp$val), min_digits(act$val, exp$val) ) msg_act <- sprintf( "Actual comparison: %s %s %s", num_exact(act$val, digits), actual_op, num_exact(exp$val, digits) ) if (is.na(diff)) { msg_diff <- NULL } else { msg_diff <- sprintf( "Difference: %s %s 0", num_exact(diff, digits), actual_op ) }

DavisVaughan · 2025-10-06T14:22:46Z

R/expect-condition.R

    if (!is.null(msg)) {
-      return(fail(msg, info = info, trace = act$cap[["trace"]]))
+      fail(msg, info = info, trace = act$cap[["trace"]])
+    } else {
+      pass()
    }


My brain really wants

if (is.null(msg)) { pass() } else { fail(msg, info = info, trace = act$cap[["trace"]]) }

i.e. always pass() first then fail()

I'm actually starting to think it pass() should always be last because if there are multiple conditions that need to be satisfied in order to pass, you want to check each of them in turn.

I like seeing pass() in the if branch and fail() in the else branch (unless it is a series of fail, fail, fail, pass) and there are a lot of places we don't do that. Up to you.

yea i noted that as my exception here

DavisVaughan · 2025-10-06T14:24:00Z

R/expect-equality.R

-    return(fail(msg, info = info))
+    fail(msg, info = info)
+  } else {
+    pass()


flip pass fail order?

DavisVaughan · 2025-10-06T14:25:09Z

R/expect-inheritance.R

-    return(fail(msg))
+    ))
+  } else {
+    pass()


flip pass fail order?

DavisVaughan · 2025-10-06T14:25:54Z

R/expect-inheritance.R

-        )
-        return(fail(msg))
-      }
+      pass()


Pretty elegant fail, fail, fail, pass fallthrough

This is why!

DavisVaughan · 2025-10-06T14:27:26Z

R/expect-known.R

+    pass()
  }


expect_file_unchanged_() is a good example of what I was saying where I thought these functions could be used for their side effects but not their return values. i.e. caller is in charge of returning invisible(act$val) which feels right to me

DavisVaughan · 2025-10-06T14:29:37Z

R/expect-self-test.R

 #' @param message Check that the failure message matches this regexp.
 #' @param ... Other arguments passed on to [expect_match()].
 #' @export
 expect_success <- function(expr) {


What is this supposed to return? And expect_failure()? Nothing I guess? There's not really an actual input?

DavisVaughan · 2025-10-06T14:32:44Z

R/expect-shape.R

  check_dots_empty()
  check_exclusive(nrow, ncol, dim)
  act <- quasi_label(enquo(object))



expect_shape() is a place where it would be worth considering extracting out expect_shape_() that is purely about the side effect part.

It may make the early returns simpler because you don't have to worry about consistently returning invisible(act$val) in 3 separate places

Something like

expect_shape = function(object, ..., nrow, ncol, dim) { check_dots_empty() check_exclusive(nrow, ncol, dim) act <- quasi_label(enquo(object)) expect_shape_(act, ...) invisible(act$val) }

I just tried this (since I think it was worth a shot) but I don't think it nets out as much of an improvement — it simplifies the early returns but you now have to pass trace_env to fail() and call to check_....

But I eliminated the early returns another way.

DavisVaughan · 2025-10-06T14:34:20Z

R/expect-that.R

 #'   act_n <- length(act$val)
 #'   if (act_n != n) {
-#'     msg <- sprintf("%s has length %i, not length %i.", act$lab, act_n, n)
-#'     return(fail(msg))
+#'     fail(sprintf("%s has length %i, not length %i.", act$lab, act_n, n))
+#'   } else {
+#'     pass()
 #'   }


For an example, I think it is probably better practice to show the pass() first then the fail()?

I can't quite describe why that feels better to me, but it does. Happy path first?

This comment was marked as outdated.

Sign in to view

hadley added 2 commits October 3, 2025 16:56

Always return act$val

c219d63

Polishing/fixes

7bb8bff

hadley requested a review from DavisVaughan October 3, 2025 22:13

hadley added 2 commits October 3, 2025 17:20

Fix mistake

c4a6e27

Update a few more

c7e1f2d

hadley added 3 commits October 6, 2025 08:02

Push through more expectations

87377ac

Fix remaining failure returns

832e5d3

Better

be37cf5

hadley added 7 commits October 6, 2025 08:12

Add news bullet

9bba39e

Polish expect_named()

a530c63

Polish expectation tests

6c40b18

Align expect_file_unchanged_

22d01a9

Fix typo

f3126ca

Polish vignette

7397403

Eliminate intermediate variable

bf46420

DavisVaughan approved these changes Oct 6, 2025

View reviewed changes

hadley added 7 commits October 6, 2025 09:51

Rework expect_match_()

0281227

Eliminate early returns

fb41736

Refactor expect_comparison()

963e37b

More to expect_match()

5d8887d

Remove early return in example

8294b85

Be more explicit about returned values

1a048b4

Update advice

843e34d

hadley merged commit dba0ef1 into main Oct 6, 2025
13 checks passed

hadley deleted the better-composition branch October 6, 2025 17:23

		@@ -1,5 +1,6 @@
		# testthat (development version)

		* Expectations now consistently return the value of the first argument, regardless of whether the expectation succeeds or fails. The primary exception are `expect_message()` and friends which will return the condition. This shouldn't affect existing tests, but will make failures clearer when you chain together multiple expectations (#2246).

Improved composition #2250

Improved composition #2250

Conversation

hadley commented Oct 3, 2025

Uh oh!

This comment was marked as outdated.

hadley commented Oct 3, 2025

Uh oh!

hadley commented Oct 5, 2025

Uh oh!

hadley commented Oct 6, 2025

Uh oh!

DavisVaughan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!