New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unittest times out if failure occurs during async callback while waiting for a future #20153

Closed
stereotype441 opened this Issue Jul 22, 2014 · 8 comments

Comments

Projects
None yet
4 participants
@stereotype441
Member

stereotype441 commented Jul 22, 2014

I am trying to use the unittest module to implement integration tests that open a connection to a subprocess, exchange data with it, and check that the appropriate responses are received from the subprocess.

Since there will be many integration tests, I've placed the code for communicating with the subprocess in a separate class. This class provides convenience functions to allow the tests to be written at a reasonably high level; those convenience functions generally do their work using asynchronous callbacks (which are guarded by expectAsync()), and then use the Completer class to signal to the test when it is time to proceed to the next step in the communication protocol.

The problem I'm running into is that if the helper class detects an error condition (using expect()) while the test is waiting on a future, the unit test doesn't stop--it continues to execute until it times out. Then, when the unit test times out, only the timeout is reported; the error condition is lost.

Here is a short contrived example that illustrates the problem:

  import 'dart:async';
  import 'package:unittest/unittest.dart';

  main() {
    test('foo', () {
      var completer = new Completer();
      new Timer(new Duration(seconds: 1), expectAsync(() {
        expect(false, isTrue);
        completer.complete();
      }));
      return completer.future;
    });
  }

This produces the result:

  ERROR: foo
    Test timed out after 120 seconds.

  0 PASSED, 0 FAILED, 1 ERRORS
  Unhandled exception:
  Exception: Some tests failed.
  #­0 SimpleConfiguration.onDone (package:unittest/src/simple_configuration.dart:189:9)
  #­1 _completeTests (package:unittest/unittest.dart:487:17)
  #­2 _runTest (package:unittest/unittest.dart:436:19)
  #­3 _nextTestCase (package:unittest/unittest.dart:376:11)
  #­4 _runTest.<anonymous closure> (package:unittest/unittest.dart:452:24)
  #­5 _createTimer.<anonymous closure> (dart:async-patch/timer_patch.dart:11)
  #­6 _handleTimeout (dart:io/timer_impl.dart:292)
  #­7 _handleTimeout (dart:io/timer_impl.dart:301)
  #­8 _handleTimeout (dart:io/timer_impl.dart:301)
  #­9 _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:124)

I would have expected for the test to terminate as soon as "expect(false, isTrue);" executed, with a stacktrace of that failure.

Note that I can work around the problem by replacing the line "return completer.future;" with "completer.future.then(expectAsync((_) {}));". But I'd rather not do this since it's counterintuitive.

@stereotype441

This comment has been minimized.

Show comment
Hide comment
Member

stereotype441 commented Jul 22, 2014

Added Area-Pkg, Pkg-Unittest labels.

@DartBot

This comment has been minimized.

Show comment
Hide comment
@DartBot

DartBot Jul 23, 2014

This comment was originally written by @zoechi


You shouldn't need expectAsync here because you return a Future.
As far as I know it is better to use
    new Future.delayed(new Duration ...
instead of new Timer(
new Future.delayed respects zones and the error handling is better when the delayed function throws.
One disadvantage of Future.delayed is that it can not be cancelled.
None of this changes does fix your issue though.

This way would probably allow you to verify async execution similar to a completer

main() {
  test('foo', () {
    var done = expectAsync((){});
    new Future.delayed(new Duration(seconds: 1), () {
      expect(false, isTrue);
      done();
    });
  });
}

this worked too

main() {
  test('foo', () {
    var completer = new Completer();
    new Future.delayed(new Duration(seconds: 1), () {
      expect(false, isTrue);
      completer.complete();
    }).catchError((e) => completer.completeError(e));
    return completer.future;
  });
}

DartBot commented Jul 23, 2014

This comment was originally written by @zoechi


You shouldn't need expectAsync here because you return a Future.
As far as I know it is better to use
    new Future.delayed(new Duration ...
instead of new Timer(
new Future.delayed respects zones and the error handling is better when the delayed function throws.
One disadvantage of Future.delayed is that it can not be cancelled.
None of this changes does fix your issue though.

This way would probably allow you to verify async execution similar to a completer

main() {
  test('foo', () {
    var done = expectAsync((){});
    new Future.delayed(new Duration(seconds: 1), () {
      expect(false, isTrue);
      done();
    });
  });
}

this worked too

main() {
  test('foo', () {
    var completer = new Completer();
    new Future.delayed(new Duration(seconds: 1), () {
      expect(false, isTrue);
      completer.complete();
    }).catchError((e) => completer.completeError(e));
    return completer.future;
  });
}

@scheglov

This comment has been minimized.

Show comment
Hide comment
@scheglov

scheglov Jul 23, 2014

Contributor

I don't understand how https://codereview.chromium.org/416563002/ will fix the problem.
Why not use try/finally block to complete the Completer?

main() {
  test('foo', () {
    var completer = new Completer();
    new Timer(new Duration(seconds: 1), expectAsync(() {
      try {
        expect(false, isTrue);
      } finally {
        completer.complete();
      }
    }));
    return completer.future;
  });
}

Contributor

scheglov commented Jul 23, 2014

I don't understand how https://codereview.chromium.org/416563002/ will fix the problem.
Why not use try/finally block to complete the Completer?

main() {
  test('foo', () {
    var completer = new Completer();
    new Timer(new Duration(seconds: 1), expectAsync(() {
      try {
        expect(false, isTrue);
      } finally {
        completer.complete();
      }
    }));
    return completer.future;
  });
}

@stereotype441

This comment has been minimized.

Show comment
Hide comment
@stereotype441

stereotype441 Jul 23, 2014

Member

Just to clarify: I agree that the solutions suggested in comments 2 and 3 would fix my contrived example. Unfortunately they don't address the real-world situation where this bug came up (the analysis server integration tests). They reason they don't work for the analysis server integration tests is that in the real-world tests, instead of the asynchronous event happening in a timer, the asynchronous event is a particular kind of response we're expecting to receive from the analysis server subprocess. To make matters more difficult, the set of possible responses we might be waiting to receive from the subprocess varies from test to test; keeping track of the completers for all of these anticipated responses would be a lot of bookkeeping, and making sure to complete all of them in the event of every possible error would be easy to get wrong. It seemed like it would be better if the unittest framework could just abort the test if an error occurs during a callback (and as far as I can tell that seems to be the design intent of unittest--it simply doesn't work in the limited case where the test returns a future and that future has not yet completed).

The reason that https://codereview.chromium.org/416563002/ works around the problem is that it changes the test from one that returns a future to one that returns void (essentially it's the same workaround I described in the last two sentences of the bug report). This works because when the test returns void, the unit test framework (see unittest.TestCase._run()) immediately sets _testComplete to a new Completer and then starts waiting for all callbacks to complete (including the one that the workaround artificially introduced). If any callback experiences a failure while it's waiting, then _fail() will call _complete(), which will complete the completer and the test will abort. By contrast, without the workaround, _run() waits until the future returned by the test completes before setting _testComplete. As a result, if an asynchronous error happens during a callback that's unrelated to the future while _run() is waiting for the future returned by the test, there's no completer for _complete() to complete, so the test keeps running until it times out.

(Note: further discussion of https://codereview.chromium.org/416563002/ probably would be best to do over to the code review website)

Member

stereotype441 commented Jul 23, 2014

Just to clarify: I agree that the solutions suggested in comments 2 and 3 would fix my contrived example. Unfortunately they don't address the real-world situation where this bug came up (the analysis server integration tests). They reason they don't work for the analysis server integration tests is that in the real-world tests, instead of the asynchronous event happening in a timer, the asynchronous event is a particular kind of response we're expecting to receive from the analysis server subprocess. To make matters more difficult, the set of possible responses we might be waiting to receive from the subprocess varies from test to test; keeping track of the completers for all of these anticipated responses would be a lot of bookkeeping, and making sure to complete all of them in the event of every possible error would be easy to get wrong. It seemed like it would be better if the unittest framework could just abort the test if an error occurs during a callback (and as far as I can tell that seems to be the design intent of unittest--it simply doesn't work in the limited case where the test returns a future and that future has not yet completed).

The reason that https://codereview.chromium.org/416563002/ works around the problem is that it changes the test from one that returns a future to one that returns void (essentially it's the same workaround I described in the last two sentences of the bug report). This works because when the test returns void, the unit test framework (see unittest.TestCase._run()) immediately sets _testComplete to a new Completer and then starts waiting for all callbacks to complete (including the one that the workaround artificially introduced). If any callback experiences a failure while it's waiting, then _fail() will call _complete(), which will complete the completer and the test will abort. By contrast, without the workaround, _run() waits until the future returned by the test completes before setting _testComplete. As a result, if an asynchronous error happens during a callback that's unrelated to the future while _run() is waiting for the future returned by the test, there's no completer for _complete() to complete, so the test keeps running until it times out.

(Note: further discussion of https://codereview.chromium.org/416563002/ probably would be best to do over to the code review website)

@stereotype441

This comment has been minimized.

Show comment
Hide comment
@stereotype441

stereotype441 Jul 24, 2014

Member

Ok, now that I've investigated this further, I think I see how to fix it.


Added Started label.

Member

stereotype441 commented Jul 24, 2014

Ok, now that I've investigated this further, I think I see how to fix it.


Added Started label.

@stereotype441

This comment has been minimized.

Show comment
Hide comment
Member

stereotype441 commented Jul 24, 2014

Set owner to @stereotype441.

@stereotype441

This comment has been minimized.

Show comment
Hide comment
@stereotype441

stereotype441 Aug 7, 2014

Member

Fixed in revision 39013.


Added Fixed label.

Member

stereotype441 commented Aug 7, 2014

Fixed in revision 39013.


Added Fixed label.

@DartBot

This comment has been minimized.

Show comment
Hide comment
@DartBot

DartBot Jun 5, 2015

This issue has been moved to dart-lang/test#256.

DartBot commented Jun 5, 2015

This issue has been moved to dart-lang/test#256.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment