Fix unresolved ports negative test

Fix unresolved ports negative test

Committed: https://code.google.com/p/dart/source/detail?r=15215

[Tom Ball, 2012-11-21 18:24:58]

With pending CL 11413101, which no longer calls exit(255) when an isolate has an
unhandled exception, the negative test fails with:

unittest-suite-wait-for-done
FAIL: Message chain with unresolved ports
  Expected: false
       but: was <true>.
  
  #0      DefaultFailureHandler.fail
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/src/expect.dart:86:5)
  #1      DefaultFailureHandler.failMatch
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/src/expect.dart:90:9)
  #2      expect
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/src/expect.dart:55:29)
  #3      baseTest.<anonymous closure>.<anonymous closure>
(file:///usr/local/google/home/tball/src/dart-all/dart/tests/isolate/unresolved_ports_test.dart:43:13)
  #4      _SpreadArgsHelper.invoke2.invoke2.<anonymous closure>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:445:29)
  #5      guardAsync
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:766:19)
  

0 PASSED, 1 FAILED, 0 ERRORS
Unhandled exception:
Exception: Some tests failed.
#0      Configuration.onDone
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/src/config.dart:130:7)
#1      _completeTests
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:835:17)
#2      _nextBatch._nextBatch
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:818:17)
#3      _nextTestCase
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:678:14)
#4      _registerException
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:794:18)
#5      guardAsync
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:768:23)
#6      _SpreadArgsHelper.invoke2.invoke2
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:441:22)
#7      _ReceivePortImpl._handleMessage (dart:isolate-patch:37:92)

It could be a problem with my CL -- would you mind patching it into your client
and see if it works as expected?  Dying isolates now log the error and quietly
close, like Java threads with uncaught exceptions do.  Embedders can define a
Dart_IsolateUnhandledExceptionCallback callback, which will get called with the
exception.  Finally, the two dart:isolate:spawn* functions now have an optional
unhandled exception callback for exceptions that are called during execution
(not compilation).  If your test is correct but needs to capture that an
unhandled exception/isolate death  event happened, this callback may help.

This design is a draft, as we expect feedback from developers like yourself
before it's finalized.

[gram, 2012-11-21 18:34:08]

https://codereview.chromium.org/11415101/diff/2001/tests/isolate/unresolved_p...
File tests/isolate/unresolved_ports_negative_test.dart (right):

https://codereview.chromium.org/11415101/diff/2001/tests/isolate/unresolved_p...
tests/isolate/unresolved_ports_negative_test.dart:12:
positive_test.baseTest(failForNegativeTest: true);
I'm not sure why this is any better than just marking the test as skipped with
an issue number in the status file. It seems that by doing this you are going to
make the test always pass even though it isn't testing what it should do, and it
could fly under the radar, whereas if it is in the status file at least it is
called out explicitly.

[Siggi Cherem (dart-lang), 2012-11-21 18:35:51]

On 2012/11/21 18:24:58, Tom Ball wrote:
> With pending CL 11413101, which no longer calls exit(255) when an isolate has
an
> unhandled exception, the negative test fails with:
> 
> unittest-suite-wait-for-done
> FAIL: Message chain with unresolved ports
>   Expected: false
>        but: was <true>.
>   
>   #0      DefaultFailureHandler.fail
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/src/expect.dart:86:5)
>   #1      DefaultFailureHandler.failMatch
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/src/expect.dart:90:9)
>   #2      expect
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/src/expect.dart:55:29)
>   #3      baseTest.<anonymous closure>.<anonymous closure>
>
(file:///usr/local/google/home/tball/src/dart-all/dart/tests/isolate/unresolved_ports_test.dart:43:13)
>   #4      _SpreadArgsHelper.invoke2.invoke2.<anonymous closure>
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:445:29)
>   #5      guardAsync
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:766:19)
>   
> 
> 0 PASSED, 1 FAILED, 0 ERRORS
> Unhandled exception:
> Exception: Some tests failed.
> #0      Configuration.onDone
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/src/config.dart:130:7)
> #1      _completeTests
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:835:17)
> #2      _nextBatch._nextBatch
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:818:17)
> #3      _nextTestCase
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:678:14)
> #4      _registerException
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:794:18)
> #5      guardAsync
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:768:23)
> #6      _SpreadArgsHelper.invoke2.invoke2
>
(file:///usr/local/google/home/tball/src/dart-all/dart/pkg/unittest/lib/unittest.dart:441:22)
> #7      _ReceivePortImpl._handleMessage (dart:isolate-patch:37:92)
> 
> It could be a problem with my CL -- would you mind patching it into your
client
> and see if it works as expected?  Dying isolates now log the error and quietly
> close, like Java threads with uncaught exceptions do.  Embedders can define a
> Dart_IsolateUnhandledExceptionCallback callback, which will get called with
the
> exception.  Finally, the two dart:isolate:spawn* functions now have an
optional
> unhandled exception callback for exceptions that are called during execution
> (not compilation).  If your test is correct but needs to capture that an
> unhandled exception/isolate death  event happened, this callback may help.
> 
> This design is a draft, as we expect feedback from developers like yourself
> before it's finalized.

Having the test failing like you see is actually correct :)

Note that this is a negative test (the file is named ..._negative_test.dart) so
it is supposed to fail. 'test.dart' understands this convention and shows the
test as "passing".

It's true that before this CL it was failing for the wrong reasons, now it is
failing where it should. I refactored the code so the positive test and the
negative test are identical without risking having them updated differently in
the future.

[Tom Ball, 2012-11-21 18:40:10]

Got it -- lgtm, and thanks for the quick response.

[Siggi Cherem (dart-lang), 2012-11-21 18:41:49]

https://codereview.chromium.org/11415101/diff/2001/tests/isolate/unresolved_p...
File tests/isolate/unresolved_ports_negative_test.dart (right):

https://codereview.chromium.org/11415101/diff/2001/tests/isolate/unresolved_p...
tests/isolate/unresolved_ports_negative_test.dart:12:
positive_test.baseTest(failForNegativeTest: true);
On 2012/11/21 18:34:09, gram wrote:
> I'm not sure why this is any better than just marking the test as skipped with
> an issue number in the status file. It seems that by doing this you are going
to
> make the test always pass even though it isn't testing what it should do, and
it
> could fly under the radar, whereas if it is in the status file at least it is
> called out explicitly.

Quite the contrary - the point here is to detect issues quickly.

The issue behind isolate negative tests is that for a long time we had isolate
tests not running correctly. We thought that they were running to completion,
but in reality nothing was being run (for instance, we never got to the point of
running the expectations on receiving some messages).

The positive test is checking the actual behavior, while this test is checking
that the positive test is running to completion.

[gram, 2012-11-21 18:46:59]

Okay, I understand. It's convoluted but there isn't really a better way right
now. LGTM.