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

Qunit 2 Style #3509

Closed
wants to merge 3 commits into from
Closed

Conversation

misteroneill
Copy link
Member

@misteroneill misteroneill commented Aug 8, 2016

Description

This updates tests to use QUnit 2.0-style assertions and test module lifecycle methods.

Specific Changes proposed

  • Move assertions from QUnit.ok() or ok() style to assert.ok() style.
  • Replace setup() with beforeEach() and teardown() with afterEach().

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors

@misteroneill misteroneill force-pushed the qunit-2-style branch 3 times, most recently from 9a6c22b to 10b4491 Compare August 8, 2016 18:21
@misteroneill misteroneill added the needs: LGTM Needs one or more additional approvals label Aug 8, 2016
@brandonocasey
Copy link
Contributor

some of these commits are from your other PR #3508. Also we should probably update qunitjs to version 2 (maybe even karma-qunit too?)

@@ -24,23 +24,23 @@ const cleanup = (item) => {
};

if (Html5.supportsNativeTextTracks()) {
QUnit.test('trackToJson_ produces correct representation for native track object', function(a) {
QUnit.test('trackToJson_ produces correct representation for native track object', function(assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice it actually changed these from a to assert 👍

Copy link
Member

Choose a reason for hiding this comment

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

neat

Copy link
Member Author

Choose a reason for hiding this comment

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

No... I did. 😆

@gkatsev gkatsev removed the needs: LGTM Needs one or more additional approvals label Aug 8, 2016
@misteroneill
Copy link
Member Author

@brandonocasey Good call on updating QUnit maybe. Also, the extra commits are due to rebasing. Once that PR is merged, I'll rebase this one again.

@misteroneill misteroneill added needs: LGTM Needs one or more additional approvals and removed in progress labels Aug 10, 2016
@brandonocasey
Copy link
Contributor

LGTM

@misteroneill misteroneill added confirmed and removed needs: LGTM Needs one or more additional approvals labels Aug 11, 2016
@gkatsev gkatsev modified the milestone: 3.12 build-improvements Aug 11, 2016
@gkatsev gkatsev added the minor This PR can be added to a minor release. It should not be added to a patch release. label Aug 11, 2016
@gkatsev
Copy link
Member

gkatsev commented Aug 11, 2016

Oops, I forgot that karma-qunitjs is being updates here as well.

@gkatsev
Copy link
Member

gkatsev commented Aug 11, 2016

Could you rebase against master?

The tool was imperfect and I had to do a lot of search/replace to fix
it. And it broke a few tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants