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

Don't ignore outstanding work on failure #1815

Merged
merged 5 commits into from
Dec 12, 2022
Merged

Conversation

natebosch
Copy link
Member

@natebosch natebosch commented Dec 10, 2022

Fixes #1512

The test invoker tracks the work done by the test, both the callback
bodies and the other work registered with the framework, and in the
typical case waits for it to complete before the test is considered
done.

Previously, any error would immediately ignore all outstanding work. In
the case of an unhandled async error, the test may still be using
resources that will be cleaned up by the tear down.

Some tests that previously would have failed quickly will now result in
a timeout. This should be uncommon, and will only impact tests that
fail anyway.

  • Stop ignoring all outstanding work on any error.
  • Explicitly ignore outstanding work for a timeout.
  • Fix some cases that run tests that do fail and then timeout. These
    tests had been manually incrementing the outstanding callback count
    and relying on the known failure to prevent a timeout. Decrement the
    outstanding callback count after the errors have been delivered.
  • More reliably decrement the outstanding work counter in a few
    situations where an exception from a test callback could cause it to
    be missed so it had to be cleaned up by the error handler.
    • Use a try/finally around callbacks which introduce their own
      outstanding work tracking zones.
    • Use a try/catch/rethrow around the body for load suites (the
      platform work including the user main method). Using a try/finally
      can cause the runner to be held open waiting for main to complete
      after a signal should have stopped tests.

Fixes #1512

The test invoker tracks the work down by the test, both the callback
bodies and the other work registered with the framework, and in the
typical case waits for it to be complete before the test is considered
done and the next work, like running teardowns, can resume.

Previously, any error would immediately ignore all outstanding work. In
the case of an unhandled async error, the test may still be using
resources that will be cleaned up by the tear down.

- Stop ignoring all outstanding work on any error.
- Explicitly ignore outstanding work for a timeout.
- More reliably decrement the outstanding work counter in a few
  situations where an exception from a test callback could cause it to
  be missed.

Previously these errors would have also caused the outstanding work to
be ignored, so it didn't matter if the decrement happened. Use a
try/finally around callbacks which introduce their own outstanding work
tracking zones. Use a try/catch/rethrow around the body for load suites
(the platform work including the user `main` method). Using a
try/finally for the LoadSuite case can cause the runner to be held open
waiting for `main` to complete after a signal should have stopped tests.
These tests use a pattern that does not show up in users tests - they
manually `addOutstandingCallback` without any plan to remove it in order
to keep the test open indefinitely until a failure. Explicitly remove
the outstanding callback in each test which adds it, after the failures
will have been delivered.
@natebosch natebosch marked this pull request as ready for review December 10, 2022 01:57
Even though the behavior changes were mostly in the Invoker this is
easier to test with the patterns used in the declarer test.
@natebosch natebosch merged commit 89a8b12 into master Dec 12, 2022
@natebosch natebosch deleted the explore-delay-fail branch December 12, 2022 17:44
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 15, 2022
These tests have some exceptions caused by missing fields on the
dynamic variable. The exceptions cause the `isolate.resume()` call to
not run, which also cause the test body to not complete.

Currently this is masked by the test runner, which immediately ignores
the test body once the first error occurs. That behavior is changing,
and a failure will not immediately end a test when the body is still
running. dart-lang/test#1815

Some of these tests are currently are expected to fail, but not to
timeout. Fix the test implementations to more reliably complete the test
body, even when the test had an error. This does not fix the tests
themselves, it maintains the current pattern of failure, even after
updating the test runner.

- Use `Stream.firstWhere` over cancelling the stream subscription after
  the first breakpoint.
- Use `Future.whenComplete` to ensure the isolate is always unpaused,
  including after an exception in the expectations.

Tested: Passes with updated test runner in https://dart-review.googlesource.com/c/sdk/+/275401/4
Change-Id: If5a7f0264c580cb38bcc1bd95c035aaf5644124b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/275787
Auto-Submit: Nate Bosch <nbosch@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Nate Bosch <nbosch@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tearDown is called before a test block completes when there are async errors
2 participants