Core: Adding promise handling on qunit callbacks: begin, moduleStart, testStart, testDone, moduleDone #1307

step2yeung · 2018-08-22T17:08:21Z

Addresses #1228

This change consist of several parts to enable QUnit callbacks to handle promises like:

  QUnit.moduleDone(function() {
    return new Promise(function(resolve) {
      // do some async things
      resolve();
    });
  });

Make advanceTaskQueue handle promises returned from each task
Each QUnit Test consist of multiple tasks, which handles setup, test execution, teardown and reporting of test results.
"advanceTaskQueue" is the function that controls the execution flow of tasks from each test. Hence, this needs to be able to handle promises that are returned by tasks.
'runLoggingCallbacks' returning a promise
'runLoggingCallbacks' is the function used to execute callback (eg. begin, moduleStart, testStart, testDone, moduleDone, done) function that is registered. This will now return a promise of the callbacks.

Note: since we are returning Promise.all(callbackPromises), it does not resolve promises in sequential order. Open to make it handle sequentially.

Update 'runLoggingCallbacks' usage to handle promises.
Since 'runLoggingCallbacks' returns a promise, begin() and finish() will have to return that promise chain. The promise will end up being waited on by 'advanceTaskQueue', and will move forward when the promise is resolved.

Note2: Have not enabled this for QUnit.on(), QUnit.log() and QUnit.done().

TODOs:

Update Docs

step2yeung · 2018-08-23T06:13:22Z

Looks like 2 cli tests is failing, will be looking into it.

Update: Two CLI test failed
(1) not ok 15 CLI Main > exit code is 1 when no tests are run
(2) not ok 19 CLI Main > filter > exit code is 1 when no tests match filter

Both failures are due to begin() using a promise to advance the processing queue.

runLoggingCallbacks( "begin", {
   totalTests: Test.count,
   modules: modulesLog
} ).then( unblockAndAdvanceQueue );

For the test exit code is 1 when no tests are run, the change caused extra error output to be displayed:

syeung-mn5:qunit syeung$ qunit test/cli/fixtures/no-tests
TAP version 13
not ok 1 global failure
  ---
  message: "No tests were run."
  severity: failed
  actual: {}
  expected: undefined
  stack: Error: No tests were run.
    at done (/Users/syeung/Documents/Development/qunit_rep/qunit/dist/qunit.js:1504:11)
    at advanceTestQueue (/Users/syeung/Documents/Development/qunit_rep/qunit/dist/qunit.js:1400:5)
    at Object.advance (/Users/syeung/Documents/Development/qunit_rep/qunit/dist/qunit.js:1356:5)
    at unblockAndAdvanceQueue (/Users/syeung/Documents/Development/qunit_rep/qunit/dist/qunit.js:3107:20)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
  ...
1..1
# pass 0
# skip 0
# todo 0
# fail 1

VS the expected output :

syeung-mn5:qunit syeung$ qunit test/cli/fixtures/no-tests
TAP version 13
not ok 1 global failure
  ---
  message: "No tests were run."
  severity: failed
  actual: null
  expected: undefined
  stack: undefined:undefined
  ...
1..1
# pass 0
# skip 0
# todo 0
# fail 1

For the test exit code is 1 when no tests match filter, the change caused extra error output to be displayed:

syeung-mn5:qunit syeung$ qunit qunit --filter 'no matches' test
required require-dep/index.js
required require-dep/module.js
TAP version 13
Unhandled Rejection: { message: 'outside of a test context',
  stack: 'Error: outside of a test context\n    at Object.<anonymous> (/some/path/wherever/unhandled-rejection.js:20:18)' }
Unhandled Rejection: Error: No tests matched the filter "!foo|bar".
    at done (/Users/syeung/Documents/Development/qunit/dist/qunit.js:1489:12)
    at advanceTestQueue (/Users/syeung/Documents/Development/qunit/dist/qunit.js:1400:5)
    at Object.advance (/Users/syeung/Documents/Development/qunit/dist/qunit.js:1356:5)
    at unblockAndAdvanceQueue (/Users/syeung/Documents/Development/qunit/dist/qunit.js:3117:20)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
Error: Process exited before tests finished running

VS the expected output :

syeung-mn5:qunit syeung$ qunit qunit --filter 'no matches' test
required require-dep/index.js
required require-dep/module.js
TAP version 13
Unhandled Rejection: { message: 'outside of a test context',
  stack: 'Error: outside of a test context\n    at Object.<anonymous> (/some/path/wherever/unhandled-rejection.js:20:18)' }
Error: Process exited before tests finished running

Not sure how to move forward, any thoughts?

jsf-clabot · 2018-09-06T18:12:37Z

All committers have signed the CLA.

platinumazure

Just leaving a few questions-- not seeing anything that stands out as a red flag, however.

test/cli/fixtures/expected/tap-outputs.js

src/test.js

test/cli/fixtures/expected/tap-outputs.js

step2yeung · 2018-09-19T16:37:19Z

@platinumazure about the parentheses being escaped, eslint throws no-useless-escape linting errors when I try to escape it. So i will keep it as it is.

trentmwillis

Overall this is looking good. I have a couple comments about order-of-execution though. Thanks for working on this!

src/core/logging.js

src/test.js

step2yeung · 2018-10-01T18:27:15Z

@platinumazure @trentmwillis addressed the feedbacks, would you be able to give it another go to see if there's any other sticking point?

src/core/logging.js

trentmwillis

Overall, looks good. I think we just need to address the Promise support issue and then we can ship. Thanks for the hard work! :)

Edit: As a quick note, I'd be inclined to drop IE 9/10 from support, but even IE 11 doesn't have Promises.

rwjblue · 2018-10-15T14:45:37Z

After talking a bit with @step2yeung about the various options here, we came up with two different paths forward:

Refactor this pull request to allow promises to be returned without rewriting all of the code to assume promises are available. This would be pretty similar to the Test.prototype.resolvePromise code that @trentmwillis mentioned inline. Ultimately, I believe that this would satisfy the requirements of this particular PR's feature, but it would leave the codebase in a somewhat worse shape (all of the promise detection would add even more cognitive complexity).
Bundle a lightweight promise implementation within the qunit.js asset (which would not be accessible outside of QUnit's own code) that we would fallback on if global Promise is not available. This would increase the payload size of the qunit.js asset by some non-trivial amount (anywhere from 3kb to 10kb as a super rough guess), but would allow us to author this feature (and refactor other promisey features that already exist) in a much more idiomatic style.

I personally think option 2 above is better for the longer term project health and generally leaves things better off for future iteration, but am pretty confident that we can make either option work if including a fallback promise lib isn't acceptable.

@stefanpenner / @trentmwillis - What do you think?

trentmwillis · 2018-10-15T15:15:34Z

@rwjblue / @step2yeung, in general, I'm not too concerned with the file size of QUnit given that it is a development tool. I agree with your opinion that option 2 is the better approach for the long-run, but would be fine with option 1 as well as I don't think it'll negatively affect the code base much if it is factored well (likely a function or two that can be swapped out with native Promise functions later).

I originally suggested an approach like option 1 as I thought it would be quicker to get landed. Introducing an extra library that has fallback behavior will require modifications to the build system and some special testing.

With that said, I leave the choice up to the implementer!

rwjblue · 2018-10-18T15:40:10Z

@step2yeung - Looks like you need to rebase against master and resolve a yarn.lock conflict...

step2yeung · 2018-10-18T16:02:10Z

@rwjblue rebased!

step2yeung · 2018-10-19T16:43:57Z

Since we don't have appyoyer or browser stack tests turned on, I tested the above on a windows machine by running yarn dev and opening up the url tests (test/index.html, test/autostart.html, etc..) on IE11 which passes.
@trentmwillis @rwjblue mind giving another look?

rwjblue

👏 looks good @step2yeung

trentmwillis

Looks good to me! One last tiny request: can we update the relevant docs to note that callbacks can return Promises (other than the Log callback)?

Also, two asides (not to keep this PR from landing):

We need to improve our code coverage reporting. The polyfill and rollup/babel make it seem like our code coverage is much worse than it actually is.
I can't wait for us to hit a support policy where async/await is available (I know we could polyfill it, but don't really want to).

src/core/logging.js

trentmwillis · 2018-11-01T16:18:07Z

Also, we should definitely add support for this to QUnit.on() as we eventually would like to deprecate the other callbacks in favor of on. This can be done in a separate PR though.

step2yeung · 2018-11-01T17:02:01Z

Created #1329 to keep track of the two issues you raised.

trentmwillis · 2018-11-01T17:17:34Z

Thank you! I know that was a lot of work, but it's appreciated; I'm excited to see how y'all use it.

I'll try to do a new minor release tomorrow morning (unlikely to get to it today).

dcombslinkedin · 2018-11-02T23:44:40Z

Way to go, @step2yeung !!!! This is awesome stuff!

Three issues came up when trying to use QUnit in an embedded mozjs context. 1. console.warn is undefined. This regressed in a recent release where we started to assume that if 'console' is defined, that 'console.warn' is as well. This is not true in SpiderMonkey (checked mozjs 68). 2. setTimeout is undefined. QUnit has historically made sure not to rely on setTimeout, but this broke when es6-promise was added in gh-1307, which assumed presence of setTimeout. The funny part is, mozjs 68 actually has native Promise support, but the polyfill is written in a way (and built in a way, with Rollup) that unconditionally builds and imports the fallback, even when it decides not to use it. It seems Rollup does not support importing files in a way that only executes the imported files conditionally (it always hoists imported files and executes imports before any other code). So, to fix this, I'm switching the polyfil library to a lighter alternative, and patching it with a conditional check on the inside rather than on the outside, so that it can be unconditionally imported by Rollup, but still only conditionally build its setTimeout-based fallback. 3. QUnit is not exported. I believe we used to have an export to the global object if no module loader was detected, but this morphed over time and we now hardcode global exports specifically for DOM global (window) and Web Worker global (self), which means it no longer worked in SpiderMonkey. Our current structure of the internal export.js file, is to provide any and all exports we can detect, which is rather unusual in the JS ecosystem, but it would also be a breaking change to stop doing this, so I'll reserve that for QUnit 3. This patch introduces a realm-agnostic global export if and only if no other export methods were applied. I suggest that in QUnit 3 we restructure this to simply always export on the global this, and then optionally also export to one module loader, trying only one after the other (mutually exclusive), which is more in line with how UMD work etc. I won't do this now yet, because I found at least one test case that breaks if we unconditionally add a global on Node.js in QUnit 2, which is our Grunt task that assumes it has control over setting the QUnit global itself. I think that's easy to fix and fine to break soon, but not until QUnit 3.

step2yeung force-pushed the promiseify-callbacks branch from a6c0add to 6711d27 Compare August 23, 2018 17:28

step2yeung force-pushed the promiseify-callbacks branch 2 times, most recently from 23b9f10 to 2537115 Compare September 6, 2018 19:24

platinumazure reviewed Sep 6, 2018

View reviewed changes

step2yeung force-pushed the promiseify-callbacks branch from 38ce4e3 to da24318 Compare September 19, 2018 16:35

trentmwillis reviewed Sep 20, 2018

View reviewed changes

src/core/logging.js Outdated Show resolved Hide resolved

src/test.js Outdated Show resolved Hide resolved

src/test.js Outdated Show resolved Hide resolved

step2yeung force-pushed the promiseify-callbacks branch from 12c6b62 to 3044ca2 Compare September 26, 2018 20:05

rwjblue reviewed Oct 1, 2018

View reviewed changes

src/core/logging.js Show resolved Hide resolved

trentmwillis requested changes Oct 8, 2018

View reviewed changes

trentmwillis mentioned this pull request Oct 9, 2018

HTML Reporter: remove test item when hidepassed is set to true #1311

Merged

step2yeung force-pushed the promiseify-callbacks branch from 3044ca2 to 0f148ce Compare October 16, 2018 21:32

step2yeung added 3 commits October 18, 2018 09:00

Core: Adding promise handling on qunit callbacks

af628bf

Tests: Fix cli tests to work with promise handling in qunit callbacks

04cfb9f

Core: handling callback promises serially

f3f43e3

step2yeung force-pushed the promiseify-callbacks branch from 58edfb5 to b59ba6f Compare October 18, 2018 16:01

step2yeung force-pushed the promiseify-callbacks branch from b59ba6f to 71d4810 Compare October 23, 2018 17:02

Adding es6-promise as a local polyfill for unsupported browsers

671f528

step2yeung force-pushed the promiseify-callbacks branch from 71d4810 to 671f528 Compare October 23, 2018 19:37

rwjblue approved these changes Oct 29, 2018

View reviewed changes

trentmwillis requested changes Nov 1, 2018

View reviewed changes

src/core/logging.js Show resolved Hide resolved

Adding documenation to qunit callbacks for handling promises

73fbe52

step2yeung force-pushed the promiseify-callbacks branch from 755682d to 73fbe52 Compare November 1, 2018 16:52

step2yeung mentioned this pull request Nov 1, 2018

Add async support for QUnit.on() callback #1329

Closed

trentmwillis approved these changes Nov 1, 2018

View reviewed changes

trentmwillis merged commit b1a2552 into qunitjs:master Nov 1, 2018

step2yeung mentioned this pull request Nov 6, 2018

Prevent errors for skipped tests when using RSVP.Promise as Promise polyfill. emberjs/ember-qunit#381

Merged

rwjblue mentioned this pull request Feb 13, 2019

testStart callback does not wait on returned Promise. #1374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Adding promise handling on qunit callbacks: begin, moduleStart, testStart, testDone, moduleDone #1307

Core: Adding promise handling on qunit callbacks: begin, moduleStart, testStart, testDone, moduleDone #1307

step2yeung commented Aug 22, 2018 •

edited

Loading

step2yeung commented Aug 23, 2018 •

edited

Loading

jsf-clabot commented Sep 6, 2018 •

edited

Loading

platinumazure left a comment

step2yeung commented Sep 19, 2018

trentmwillis left a comment

step2yeung commented Oct 1, 2018

trentmwillis left a comment •

edited

Loading

rwjblue commented Oct 15, 2018

trentmwillis commented Oct 15, 2018

rwjblue commented Oct 18, 2018

step2yeung commented Oct 18, 2018

step2yeung commented Oct 19, 2018

rwjblue left a comment

trentmwillis left a comment

trentmwillis commented Nov 1, 2018

step2yeung commented Nov 1, 2018

trentmwillis commented Nov 1, 2018

dcombslinkedin commented Nov 2, 2018

Core: Adding promise handling on qunit callbacks: begin, moduleStart, testStart, testDone, moduleDone #1307

Core: Adding promise handling on qunit callbacks: begin, moduleStart, testStart, testDone, moduleDone #1307

Conversation

step2yeung commented Aug 22, 2018 • edited Loading

step2yeung commented Aug 23, 2018 • edited Loading

jsf-clabot commented Sep 6, 2018 • edited Loading

platinumazure left a comment

Choose a reason for hiding this comment

step2yeung commented Sep 19, 2018

trentmwillis left a comment

Choose a reason for hiding this comment

step2yeung commented Oct 1, 2018

trentmwillis left a comment • edited Loading

Choose a reason for hiding this comment

rwjblue commented Oct 15, 2018

trentmwillis commented Oct 15, 2018

rwjblue commented Oct 18, 2018

step2yeung commented Oct 18, 2018

step2yeung commented Oct 19, 2018

rwjblue left a comment

Choose a reason for hiding this comment

trentmwillis left a comment

Choose a reason for hiding this comment

trentmwillis commented Nov 1, 2018

step2yeung commented Nov 1, 2018

trentmwillis commented Nov 1, 2018

dcombslinkedin commented Nov 2, 2018

step2yeung commented Aug 22, 2018 •

edited

Loading

step2yeung commented Aug 23, 2018 •

edited

Loading

jsf-clabot commented Sep 6, 2018 •

edited

Loading

trentmwillis left a comment •

edited

Loading