dart-lang / test

A library for writing unit tests in Dart.
https://pub.dev/packages/test
495 stars 213 forks source link

isNot matcher is not negating the result of a wraped matcher #1637

Open Hu1buerger opened 2 years ago

Hu1buerger commented 2 years ago

This issue was originally reported on flutter/flutter as https://github.com/flutter/flutter/issues/94170

Steps to Reproduce

  1. Use this test

pubspec.yaml as

dev_dependencies:
  flutter_test:
    sdk: flutter
main(){
  test("given a async closure that dosnt throw, then it shouldnt throw", () async {
    await expectLater(() => Future.value("this dosnt throw and just returns"), isNot(throwsA(anything)));
  });

  test("samezys just with expect", () {
    expect(() => Future.value("same"), isNot(throwsA(anything)));
  });

  test("given sync funcs, the test should detect throwing", () {
    expect(() => "test", isNot(throwsA(anything)));
    expect(() => throw Error(), throwsA(anything));
  });

  test("given a throwing future, then the closure should throw", (){
    expect(() => Future.error(Error()), throwsA(anything));
  });
}

  1. Execute flutter test on the code appended.
  2. I would expect that both tests should satisfy this criterion.

What actually happens

Expected results: I expect that all test cases finish without error.

Actual results: Both async isNot tests fail with equivalent error messages.

  Expected: not throws anything
    Actual: <Closure: () => Future<String>>

  package:test_api                                    expectLater
  package:flutter_test/src/widget_tester.dart 498:10  expectLater
  test\example.dart 60:15                     main.<fn>.<fn>.<fn>
  test\example.dart 58:41                     main.<fn>.<fn>.<fn>
flutter doctor -v ``` PS C:\Users\user\Documents\Developer\medlog> flutter doctor -v [√] Flutter (Channel stable, 2.5.3, on Microsoft Windows [Version 10.0.22000.282], locale en-US) • Flutter version 2.5.3 at C:\Users\user\Documents\flutter • Upstream repository https://github.com/flutter/flutter.git • Framework revision 18116933e7 (6 weeks ago), 2021-10-15 10:46:35 -0700 • Engine revision d3ea636dc5 • Dart version 2.14.4 [√] Android toolchain - develop for Android devices (Android SDK version 31.0.0) • Android SDK at C:\Users\user\AppData\Local\Android\Sdk • Platform android-31, build-tools 31.0.0 • ANDROID_SDK_ROOT = C:\Users\user\AppData\Local\Android\Sdk • Java binary at: C:\Program Files\Android\Android Studio\jre\bin\java • Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7249189) • All Android licenses accepted. [√] Chrome - develop for the web • Chrome at C:\Program Files\Google\Chrome\Application\chrome.exe [√] Android Studio (version 2020.3) • Android Studio at C:\Program Files\Android\Android Studio • Flutter plugin can be installed from: https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7249189) [√] IntelliJ IDEA Ultimate Edition (version 2020.3) • IntelliJ at C:\Program Files\JetBrains\IntelliJ IDEA 2020.3 • Flutter plugin can be installed from: https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: https://plugins.jetbrains.com/plugin/6351-dart [√] VS Code (version 1.62.2) • VS Code at C:\Users\user\AppData\Local\Programs\Microsoft VS Code • Flutter extension version 3.28.0 [√] Connected device (1 available) • Chrome (web) • chrome • web-javascript • Google Chrome 95.0.4638.69 • No issues found! ```
Hu1buerger commented 2 years ago

I have seen #539 and already switched to returnsNormally.

Would it be possible to make isNot aware of async matchers in, it just throws an error if it detects a closure instead of a value?

jakemac53 commented 2 years ago

There may also be cases where it is valid to pass a closure to isNot (you could be checking the function type, etc).

We could possibly use printOnFailure though and log a warning message that way, so it only warns if the test fails? The valid use cases should be few and far between.

Hu1buerger commented 2 years ago

@jakemac53 I traced the callstack for the first test (s.a.) to https://github.com/dart-lang/test/blob/a0f260c17f3f066b61fa707198e7790894d4b58e/pkgs/test_api/lib/src/expect/throws_matcher.dart#L97 but cannot seem to get the line after that L98 to catch in the debugger. Before that happens I get the test failure and am imagining that this happens due to throwsMatcher running async compared to isNot. Is that just me?

And could you elaborate on what exactly are you thinking about as valid args for isNot.

Hu1buerger commented 2 years ago

@jakemac53 were you able to reproduce my last comment

jakemac53 commented 2 years ago

Sorry, lost track of this issue.

but cannot seem to get the line after that L98 to catch in the debugger. Before that happens I get the test failure and am imagining that this happens due to throwsMatcher running async compared to isNot. Is that just me?

Right, the implementation of isNot just delegates to .matches of the wrapped matcher. Async matchers are a massive hack basically and their .matches just returns true (unless they are given a non-future). Then they throw (or not) later on after the async thing has completed.

And could you elaborate on what exactly are you thinking about as valid args for isNot.

Technically you could be checking whether a function is exactly equal to an expected function (or not, in this case). The use cases seem few and far between but its technically something that would be valid to do.

Hu1buerger commented 2 years ago

Should the isNot matcher even handle async matchers? Would there be a valid use case where the isNot matcher wraps an asyncMatcher and still be valid?

jakemac53 commented 2 years ago

Should the isNot matcher even handle async matchers? Would there be a valid use case where the isNot matcher wraps an asyncMatcher and still be valid?

The isNot matcher can't even check - the AsyncMatcher class is a part of package:test (its weird for sure...but it is what it is).