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

Core: Fix late onerror handling #1629

Merged
merged 2 commits into from
Jul 3, 2021
Merged

Core: Fix late onerror handling #1629

merged 2 commits into from
Jul 3, 2021

Commits on Jun 28, 2021

  1. Test: Change tests to not rely on "double done" hack

    The next commit in this branch for qunitjs#1511, will disallow adding
    tests if `QUnit.done()` and `runEnd` have already happened, thus
    leading these hacks to fail as follows:
    
    ````
    Running "qunit:all" (qunit) task
    Testing http://localhost:4000/test/index.html
    […]
    Testing http://localhost:4000/test/module-skip.html ....
    Error: Unexpected new test after the run already ended
        at new Test (http://localhost:4000/qunit/qunit.js:2206:13)
    ^C
    ```
    
    In addition, due to a known issue in grunt-contrib-qunit, these would
    also indefinitely hack instead of actually failing.
    
    Ref gruntjs/grunt-contrib-qunit#178.
    Ref qunitjs#1377.
    Ref qunitjs#1511.
    Krinkle committed Jun 28, 2021
    Configuration menu
    Copy the full SHA
    ceec61b View commit details
    Browse the repository at this point in the history

Commits on Jul 3, 2021

  1. Core: Fix late onerror handling

    == Background ==
    
    Previously, QUnit.onError and QUnit.onUnhandledRejection could report
    global errors by synthesizing a new test, even after a run has ended.
    
    This is problematic when an errors ocurrs after all modules (and their
    hooks) have finished, and the overall test run has ended.
    
    The most immediate problem is that hooks having finished already,
    means it is illegal for a new test to start since "after" has already
    run. To protect against such illegal calls, the hooks object is
    emptied internally, and this new test causes an internal error:
    
    ```
    TypeError: Cannot read property 'length' of undefined
    ```
    
    This is not underlying problem though, but rather our internal
    safeguard working as intended. The higher-level problem is that there
    is no appropiate way to report a late error as a test since the run
    has already ended. The `QUnit.done()` callbacks have run, and
    the `runEnd` event has been emitted.
    
    == Approach ==
    
    Instead of trying to report (late) errors as a test, only print them
    to `console.warn()`, which goes to stderr in Node.js. For the CLI, also
    remember that uncaught errors were found and use that to make sure we
    don't change exitCode back to zero (e.g. in case we have an uncaught
    error after the last test but before our `runEnd` callback is called).
    
    == Changes ==
    
    * Generalise `QUnit.onUnhandledRejection` and re-use it for
      `window.onerror` (browser), and uncaught exceptions (CLI).
    
    * Fix broken use of `QUnit.onError` in `process.on( "uncaughtException" )`.
      This was passing the wrong parameters. Use the new onUncaughtException
      method instead.
    
    * Clarify that `QUnit.onError` is only for `window.onerror`. For now,
      keep its strange non-standard signature as-is (with the custom object
      parameter), but document this and its return value.
    
    * Remove the unused "..args" from `QUnit.onError`. This was only ever
      passed from one of our unit tests to give one extra argument (a
      string of "actual"), which then ended up passed as "actual" parameter
      to `pushFailure()`. We never used this in the actual onError binding,
      so remove this odd variadic construct for now.
    
    * Change `ProcessingQueue#done`, which is in charge of reporting
      the "No tests were run" error, to no longer rely on the way that
      `QUnit.onError` previously queued a late test.
    
      The first part of this function may run twice (same as before, once
      after an empty test run, and one more time after the synthetic
      test has finished and the queue is empty again). Change this so that
      we no longer assign `finished = true` in that first part. This means
      we will still support queueing of this one late test. But, since the
      quueue is empty, we do need to call `advance()` manually as otherwise
      it'd never get processed.
    
      Previously, `finished = true` was assigned first, which meant that
      `QUnit.onError` was adding a test under that condition. But this
      worked anyway because `Test#queue` internally had manual advancing
      exactly for this use case, which is also where we now emit a
      deprecation warning (to become an error in QUnit 3). Note that using
      this for anything other than the "No tests run" error was already
      unreliable since generally runEnd would have been emitted already.
      The "No tests run" test was exactly done from the one sweet spot
      where it was (and remains) safe because that threw an error and thus
      prevented runEnd from being emitted.
    
    Fixes qunitjs#1377.
    Ref qunitjs#1322.
    Ref qunitjs#1446.
    Krinkle committed Jul 3, 2021
    Configuration menu
    Copy the full SHA
    14a0e11 View commit details
    Browse the repository at this point in the history