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

Upgrade qunitjs to 2.x #123

Merged
merged 1 commit into from
Mar 29, 2017
Merged

Upgrade qunitjs to 2.x #123

merged 1 commit into from
Mar 29, 2017

Conversation

mucaho
Copy link

@mucaho mucaho commented May 5, 2016

This is an in-progress implementation that fixes #122 .

  • Update bridge between phantomjs and qunit
  • Update tests as stated in upgrade guide
  • Change devDependency qunitjs from version 2.0.0-rc1 to 2.3.0
  • Update documentation to reflect the change
  • Things I may have missed

@jzaefferer
Copy link
Member

Good start, thanks @mucaho!

@leobalter should we add autorun to the QUnit.config docs or mention it in the upgrade guide? How should we deal with that here?

@leobalter
Copy link
Member

autorun is not part of QUnit 2.0. It wasn't a documented feature on 1.x as well. We discussed this on the removals for 2.0

QUnit.config.autostart should work and it's documented.

@jzaefferer
Copy link
Member

@mucaho can you remove/replace any usage of autorun?

@mucaho
Copy link
Author

mucaho commented May 14, 2016

Ok, removed autorun.

However, the test reporter seems to rely on serial execution order of tests. If QUnit will call interleaved callbacks for test cases, that could mess up reporting. (Probably most noticeable with verbose set and many asynchronous tests).
Specifically, tasks/qunit.js#failedAssertions (here, here, here, here).

One simple approach would be to match the QUnit.log callbacks to the appropriate QUnit.testStart & QUnit.testDone callbacks by comparing the name and module fields. However, this may in rare cases not work with asynchronous tests that have the same name.

@jzaefferer
Copy link
Member

You removed the "Run tests serially, not in parallel." comment, is that where you got the idea that something is running in parallel? I'm pretty sure QUnit runs all tests in sequence, not in parallel. Or is there something else?

@stevenbenner
Copy link

QUnit 2.0.0 has been released and is up on npm now. 2.0.0 release notes

@jzaefferer
Copy link
Member

@mucaho could you update this?

@matzetronic
Copy link

matzetronic commented Jul 14, 2016

Hi,
can you give us an update of this issue? I wrote my tests already in QUnit 2.0 and I also have the issue #129 so I thought this could be related to this and how QUnit test results are reported from QUnit 2.0 tests to the 1.20 QUnit used by grunt-contrib-qunit..

@Arkni
Copy link
Member

Arkni commented Jul 14, 2016

@matzetronic the current version of this plugin (v1.2.0) should work just fine with any version of QUnit. There is only one requirement of using versions of QUnit >= 1.23.0 in order to use the CLI flags --seed (not released yet) and --modules.

As for #129, could you please provide the steps + source code to reproduce (please put them in #129)

@vogdb
Copy link

vogdb commented Nov 11, 2016

Hiii! I'm sorry to bother but what is the status here?

@jsf-clabot
Copy link

jsf-clabot commented Mar 29, 2017

CLA assistant check
All committers have signed the CLA.

@mucaho mucaho force-pushed the upgrade branch 2 times, most recently from 3c1cd20 to b286d08 Compare March 29, 2017 18:05
Update test files to use new non-global QUnit syntax.
Update bridge between this grunt plugin and phantomjs:
* Remove deprecated autorun QUnit option.
Update devDependency qunitjs to 2.3.0.
Update Gruntfile to account for failed tests.
@mucaho
Copy link
Author

mucaho commented Mar 29, 2017

Updated the qunitjs devDependency to ^2.3.0.
Had to adapt the noglobals test file since it's no longer possible to overwrite the error reporting function pushFailure of QUnit and thus I didn't find a way to prevent QUnit from reporting a failure. If you know of a more elegant workaround, please let me know.

@leobalter
Copy link
Member

cc @trentmwillis

@leobalter
Copy link
Member

@Arkni it looks good to me. :shipit:

@Arkni Arkni removed the in progress label Mar 29, 2017
@Arkni Arkni requested review from Arkni and removed request for Arkni March 29, 2017 22:49
@Arkni Arkni merged commit 353a7a8 into gruntjs:master Mar 29, 2017
@Arkni
Copy link
Member

Arkni commented Mar 29, 2017

@mucaho
Thanks a lot :)

And sorry for forgotten about this PR!

@mgol
Copy link

mgol commented Mar 31, 2017

@leobalter this plugin needs a major version bump now, doesn't it?

@mucaho mucaho deleted the upgrade branch March 31, 2017 14:18
@trentmwillis
Copy link
Contributor

Yes, next release should be a major version.

@leobalter
Copy link
Member

yes, please

@Arkni
Copy link
Member

Arkni commented Mar 31, 2017

We also need to remove this part of code phantomjs/bridge.js#L36-L39 before releasing. We don't need that BC check anymore.

@Arkni
Copy link
Member

Arkni commented Apr 3, 2017

@leobalter
Should we also switch to use the js-reporter events (using QUnit.on(...)) instead of the the callback QUnit.log(...), ... ?
If we do that, we will need to set the min supported version of QUnit to 2.3.0

@leobalter
Copy link
Member

@Arkni it's not necessary for now, it's still valid to use QUnit.log but it would be great if you want to try QUnit.on. The api is a bit different, thou.

@Arkni
Copy link
Member

Arkni commented Apr 3, 2017

OK then. I'll try to remove the lines I referenced in my previous comment #123 (comment) and do a release tomorrow (in case no one beat me to it :-) ).

I'll play with it and test it on some of my projects before changing any thing in this plugin.

Thanks!

@Arkni
Copy link
Member

Arkni commented Apr 4, 2017

Published to npm as 2.0.0 :)

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.

Upgrade qunitjs to 2.x