Should `fail` and `TestFailure` be exposed by scaffolding.dart?

[jamesderlin]

I have a test that looks like:

try {
  functionExpectedToFail();
  fail('Failed to fail.');
} on ExpectedException catch (e) {
  // Significant code that extracts properties of `e` and that verifies the results...
}

While it's probably possible to rewrite the test to use a more typical check(functionExpectedToFail).throws<ExpectedException>() style, there's a significant amount of code in my catch block, so I think it's more readable with the explicit try-catch.

However, fail (and the TestFailure exception it wants to throw) are not exposed by package:test/scaffolding.dart. Should they be? Is there something else I should use instead?

(And yes, I also could just explicitly throw some other exception.)

[natebosch]

I do prefer to push the check(functionExpectedToFail).throws<ExpectedException>() style.

there's a significant amount of code in my catch block, so I think it's more readable with the explicit try-catch.

Is there behavior other than checking expectations on e?

Do you have a concrete example that we could compare against the .throws<ExpectedException>() pattern?

[jamesderlin]

Is there behavior other than checking expectations on e?

I'd like to do something like:

var stopwatch = Stopwatch()..start();
try {
  functionExpectedToFail();
  fail('Failed to fail.');
} on ExpectedException catch (e) {
  check(stopwatch.elapsed).isLessThan(timeToFailThreshold);
  check(e.someProperty).deepEquals(expectedValue);
  check(someTransformation(e.someOtherProperty)).deepEquals(someOtherExpectedValue);
}

[natebosch]

I'd like to do something like:

Interesting, the stopwatch use case is not one I had predicted.

If we were to naively force this into full checks pattern I think it would look like

    var stopwatch = Stopwatch()..start();
    check(functionExpectedToFail).throws<ExpectedException>()
      ..has((_) => stopwatch.elapsed, 'elapsed time')
          .isLessThan(timeToFailThreshold)
      ..has((e) => e.someProperty, 'someProperty').deepEquals(expectedValue)
      ..has((e) => someTransformation(e.someOtherProperty),
              'some transformed property')
          .deepEquals(somOtherExpectedValue);

The planned codegen would only cover a small part of that

     check(functionExpectedToFail).throws<ExpectedException>()
       ..has((_) => stopwatch.elapsed, 'elapsed time')
           .isLessThan(timeToFailThreshold)
-      ..has((e) => e.someProperty, 'someProperty').deepEquals(expectedValue)
+      ..someProperty.deepEquals(expectedValue)
       ..has((e) => someTransformation(e.someOtherProperty),
               'some transformed property')
           .deepEquals(somOtherExpectedValue);

We could write a small utility to cover the someTransformation case which works nicely with the codegen. The implementation overhead is lower if someTransformation is already a test utility and can be directly rewritten as the extension.

       ..has((_) => stopwatch.elapsed, 'elapsed time')
           .isLessThan(timeToFailThreshold)
       ..someProperty.deepEquals(expectedValue)
-      ..has((e) => someTransformation(e.someOtherProperty),
-              'some transformed property')
-          .deepEquals(somOtherExpectedValue);
+      ..someOtherProperty.transformed.deepEquals(someOtherExpectedValue);
   });
 }
+
+extension on Subject<PropertyType> {
+  Subject<TransformedType> get transformed => context.nest(
+      'transformed', (value) => Extracted.value(someTransformation(value)));
+}

I'm less sure what to do about the stopwatch though. It's a bit of an unusual test since we don't expect code running as a test to perform the same as code running in production. If it can tolerate the perf difference anyway, it should probably handle the small overhead that the .throws check may have. I think it's probably OK for an unusual test pattern to have an unusual checks usage, so we could hold the Subject in a variable before doing the expensive deepEquals. That would make the rewrite:

    var stopwatch = Stopwatch()..start();
    var exceptionSubject = check(functionExpectedToFail).throws<ExpectedException>();
    check(stopwatch).elapsed.isLessThan(timeToFailThreshold);
    exceptionSubject
      ..someProperty.deepEquals(expectedValue)
      ..someOtherProperty.transformed.deepEquals(someOtherExpectedValue);
      
    // ...
extension on Subject<PropertyType> {
  Subject<TransformedType> get transformed => context.nest(
      'transformed', (value) => Extracted.value(someTransformation(value)));
}

I can see the appeal of writing it with the try/fail()/catch over using codegen and the extra extension on Subject utility. I've seen that pattern get implemented wrong a few times though, and I'm wary of encouraging it.

I do think that checks has a better story for this situation than expect() does. I don't think that expect would let you split up parts of the expectations into different points like checks allows with the Subject variable. You must either get the exception as a variable yourself (with a try/catch probably), or do all of the expectations in the same call to expect by chaining some .having on the TypeMatcher.

cc @jakemac53 @kevmoo for any thoughts on these patterns