Skip to content
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

Jump to bottom

Exceptions are swallowed #10511

Closed
peter-ahe-google opened this issue May 7, 2013 · 8 comments
Closed

Exceptions are swallowed #10511

peter-ahe-google opened this issue May 7, 2013 · 8 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-obsolete Closed as the reported issue is no longer relevant library-async

Comments

@peter-ahe-google
Copy link
Contributor

Consider the program and output below. The program contains an additional error, it calls a missing method "x". I had expected to see an exception and stack trace for that. Instead, I only see a stack trace for the subsequent failure.

This (and misreading a comment in dart2js) cost me a few hours debugging the wrong thing. I'm pretty sure if I had seen the swallowed exception, I would have realized my mistake earlier.

I'm tentatively pointing fingers at the unittest package, but this could just as well be a bug in dart:async, so I'm CC'ing Florian.

import 'dart:async';

import 'package:unittest/unittest.dart';

fisk() {
  new Future(() => x());
  new Future(() => 100).then(expectAsync1((x) {
    expect(x, equals(44));
  }));
}

main() {
  test('fisk', fisk);
}

Running this program on dart2js/d8 looks like this:

swallow.dart:6:20: Warning: cannot resolve x
  new Future(() => x());
                   ^
Dart file swallow.dart compiled to JavaScript: out.js
unittest-suite-wait-for-done
FAIL: fisk
  Expected: <44>
       but: was <100>.
  
  Expected: <44>
       but: was <100>.
      at Configuration.$$.Configuration.onExpectFailure$1 (out.js:3614:15)
      at _ExpectFailureHandler.$$._ExpectFailureHandler.fail$1 (out.js:3559:18)
      at $.expect (out.js:6116:20)
      at fisk_anon1.$$.fisk_anon1.call$1 (out.js:3543:7)
      at _SpreadArgsHelper.$$._SpreadArgsHelper.callback$1 (out.js:4312:26)
      at _SpreadArgsHelper_invoke1_anon.$$._SpreadArgsHelper_invoke1_anon.call$0 (out.js:4369:17)
      at $._guardAsync (out.js:6257:18)
      at _SpreadArgsHelper.$$._SpreadArgsHelper.invoke1$1 (out.js:4346:14)
      at BoundClosure$1.$$.BoundClosure$1.call$1 (out.js:4436:34)
      at _ThenFuture.$$._ThenFuture._onValue$1 (out.js:2181:26)
  

0 PASSED, 1 FAILED, 0 ERRORS
out.js:2376: Exception: Some tests failed.
        throw exception;
              ^
Exception: Some tests failed.
    at $.throwExpression (out.js:5237:11)
    at $._completeTests (out.js:6329:9)
    at $._nextBatch (out.js:6287:9)
    at _nextTestCase_anon.$$._nextTestCase_anon.call$0 (out.js:4376:7)
    at Function.$._asyncRunCallback as call$0
    at Timer_run_anon.$$.Timer_run_anon.call$0 (out.js:2368:18)
    at _IsolateContext.$$._IsolateContext.eval$1 (out.js:683:21)
    at _IsolateEvent.$$._IsolateEvent.process$0 (out.js:798:18)
    at _EventLoop.$$._EventLoop.runIteration$0 (out.js:759:12)
    at _EventLoop.$$._EventLoop._runHelper$0 (out.js:766:19)

Running this program on standalone Dart VM looks like this:

unittest-suite-wait-for-done
FAIL: fisk
  Expected: <44>
       but: was <100>.
  
  fisk.fisk.<anonymous closure> file:///Users/ahe/Dart/all/dart/swallow.dart 8:11
  _SpreadArgsHelper.invoke1.invoke1.<anonymous closure> package:unittest/unittest.dart 496:28
  _guardAsync package:unittest/unittest.dart 739:19
  _SpreadArgsHelper.invoke1.invoke1 package:unittest/unittest.dart 493:23
  _ThenFuture._sendValue dart:async 397:24
  _FutureImpl._setValue dart:async 294:26
  _FutureImpl._setOrChainValue dart:async 371:16
  _ThenFuture._sendValue dart:async 403:21
  Future.Future.<anonymous closure> dart:async 66:37
  _asyncRunCallback._asyncRunCallback dart:async 34:17
  Timer.run.<anonymous closure> dart:async 2251:21
  Timer.Timer.<anonymous closure> dart:async-patch 15:15
  

0 PASSED, 1 FAILED, 0 ERRORS
Unhandled exception:
Exception: Some tests failed.

­0 Configuration.onDone (package:unittest/src/config.dart:213:9)

­1 _completeTests (package:unittest/unittest.dart:805:17)

­2 _nextBatch (package:unittest/unittest.dart:775:21)

­3 _nextTestCase.<anonymous closure> (package:unittest/unittest.dart:673:15)

­4 _asyncRunCallback._asyncRunCallback (dart:async:34:17)

­5 _asyncRunCallback._asyncRunCallback (dart:async:44:9)

­6 Timer.run.<anonymous closure> (dart:async:2251:21)

­7 Timer.run.<anonymous closure> (dart:async:2259:13)

­8 Timer.Timer.<anonymous closure> (dart:async-patch:15:15)

­9 _Timer._createTimerHandler._handleTimeout (dart:io:6794:28)

­10 _Timer._createTimerHandler._handleTimeout (dart:io:6802:7)

­11 _Timer._createTimerHandler.<anonymous closure> (dart:io:6810:23)

­12 _ReceivePortImpl._handleMessage (dart:isolate-patch:81:92)
@lrhn
Copy link
Member

lrhn commented May 8, 2013

This is a problem with async programming and testing in general.
There is nothing listening on the faulty future, so the error should eventually be reported as uncaught.

However, if the other error happens first, and the program is aborted (all the expectAsync's have been hit, so the framework has no reason not to end it), the error will not be reported. The unittest framework actually forcibly ends the program after the first error, and your test has two errors. Change the 44 to 100, and you will see the other error.

@peter-ahe-google
Copy link
Contributor Author

This sounds like a bug in dart:async.

The outcome is obviously unacceptable, and cannot be excused away as a "general problem with async programming and testing".

Removed Area-UnitTest label.
Added Area-Library label.

@peter-ahe-google
Copy link
Contributor Author

An additional point: the unittest framework is not "forcibly" ending the program. This program runs on both dart2js and Dart VM and only the latter has an "exit" primitive.

As far as i know, the unittest framework is doing anything, it is throwing an exception from async code and closing a port. See pkg/unittest/lib/src/config.dart, the onDone method.

@gramster
Copy link
Contributor

If you are going to use Futures in a unittest, you need to return a Future from the test.

fisk() {
  new Future(() => x());
  new Future(() => 100).then(expectAsync1((x) {
    expect(x, equals(44));
  }));
}

should be:

fisk() {
  var f1 = new Future(() => x());
  var f2 = new Future(() => 100).then(expectAsync1((x) {
    expect(x, equals(44));
  }));
  return Future.wait([f1, f2]);
}

Then running with the VM should show:

ERROR: fisk
  Caught No top-level method 'x' declared.
  
  NoSuchMethodError : method not found: 'x'
  Receiver: top-level
  Arguments: [...]
  fisk.fisk.<anonymous closure> file:///tmp/junk.dart 6:29

@peter-ahe-google
Copy link
Contributor Author

Graham: people make mistakes and exceptions should not be swallowed. When you make a mistake, an error should occur.

@gramster
Copy link
Contributor

I agree in principle - if we had a global exception handler then unittest could deal with this.

@lrhn
Copy link
Member

lrhn commented Apr 30, 2014

Added Library-Async label.

@lrhn
Copy link
Member

lrhn commented Oct 7, 2014

If changing expectAsync1 to expectAsync, the test now runs and prints:

unittest-suite-wait-for-done
FAIL: fisk
  Caught No top-level method 'x' declared.
  
  NoSuchMethodError: method not found: 'x'
  Receiver: top-level
  Arguments: [...]
  dart:core-patch/errors_patch.dart 168:5 NoSuchMethodError._throwNew
  fisk.dart 6:20 fisk.<fn>
  dart:async/future.dart 118:37 Future.Future.<fn>
  dart:async/zone.dart 890:38 _rootRun
  dart:async/zone.dart 795:37 _CustomZone.run
  dart:async/zone.dart 703:17 _CustomZone.runGuarded
  dart:async/zone.dart 728:35 _CustomZone.bindCallback.<fn>
  dart:async/zone.dart 894:13 _rootRun
  dart:async/zone.dart 795:37 _CustomZone.run
  dart:async/zone.dart 703:17 _CustomZone.runGuarded
  dart:async/zone.dart 728:35 _CustomZone.bindCallback.<fn>
  dart:async-patch/timer_patch.dart 12:63 Timer._createTimer.<fn>
  dart:io/timer_impl.dart 292:19 _handleTimeout
  dart:isolate-patch/isolate_patch.dart 130:12 _RawReceivePortImpl._handleMessage

It seems the unittest package now correctly handles this case.

Added AssumedStale label.

@peter-ahe-google peter-ahe-google added Type-Defect area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-async closed-obsolete Closed as the reported issue is no longer relevant labels Oct 7, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-obsolete Closed as the reported issue is no longer relevant library-async
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gramster @lrhn @peter-ahe-google