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 QUnit to 2.0 #234

Merged
merged 2 commits into from
Aug 17, 2016
Merged

Upgrade QUnit to 2.0 #234

merged 2 commits into from
Aug 17, 2016

Conversation

trentmwillis
Copy link
Member

Need to verify what else needs to happen in ember-cli-qunit before proceeding with merging this. Also want to double check behavior of Acceptance tests, since I know of at least this issue: emberjs/ember.js#13696 which we plan to try and fix in this addon instead of in Core.

In this PR you'll notice that aside from upgrading QUnit, I also made JSHint stricter and removed the old test wrapper functions, which were primarily there to support promise-returning tests which are now supported in QUnit proper.

@trentmwillis trentmwillis changed the title [Don't Merge Yet] Upgrade QUnit to 2.0 Upgrade QUnit to 2.0 Jul 31, 2016
@trentmwillis
Copy link
Member Author

Tested this with the changes made to ember-cli-qunit against Ember Observer's test suite and all is passing! This should be safe to land now, but will require a major version bump before releasing.

screen shot 2016-07-31 at 5 03 10 pm

@elwayman02
Copy link
Contributor

@trentmwillis this needs a rebase

@trentmwillis
Copy link
Member Author

Rebased.

return module.setup().then(function() {
beforeEach() {
return module.setup(...arguments).then(() => {
Object.keys(module.context).forEach((key) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets extract this into https://github.com/rwjblue/ember-qunit/pull/237 for now. I'd prefer to just use QUnit's test context directly as mentioned in a comment over there. I definitely do not see this as a blocker either way though.

@rwjblue
Copy link
Member

rwjblue commented Aug 16, 2016

LGTM with the few tweaks/changes mentioned above. I think we want to land https://github.com/rwjblue/ember-qunit/pull/237 first though.

@rwjblue
Copy link
Member

rwjblue commented Aug 16, 2016

Landed changes to context in #238 (which closed #237). This is no longer blocked.

@trentmwillis
Copy link
Member Author

Will work on this tonight and try to bring in the necessary test adapter changes.

if (afterEach) {
afterEach.call(module.context, assert);
afterEach.apply(this, arguments);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that a promise returned from the user's specified afterEach isn't handled properly (meaning we don't block on it before calling module.teardown() just below). We don't need to block this on fixing that, but it would be nice to get it fixed in a future refactor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I'll do a separate PR for that.

@trentmwillis
Copy link
Member Author

trentmwillis commented Aug 17, 2016

@rwjblue I think this should be good to go now. I added in the adapter from the other PR.

@rwjblue
Copy link
Member

rwjblue commented Aug 17, 2016

Just landed #239 (to allow us to have a final good QUnit 1.x version first), so this will need a rebase.

@trentmwillis
Copy link
Member Author

@rwjblue rebased

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.

3 participants