Skip to content

Tests: Replace resource loader with AMD #1511

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

Closed
wants to merge 14 commits into from
Closed

Conversation

scottgonzalez
Copy link
Member

Replaces gh-1335.
Replaced by gh-1528.

This reduces the changes to only affect the accordion test suite so it's easier to discuss and make changes.

  • Simplify per-test boilerplate
  • Convert domEqual() to a real assertion
  • Replace custom QUnit.assert.close() with qunit-assert-close module
  • Move domEqual() to its own module
  • Properly update QUnit (can now be done by rebasing on master)
  • Update all tests (currently just accordion)
  • Remove globals from tests/.jshintrc
  • Figure out how to use AMD in grunt-contrib-qunit
  • Pull in domEqual() changes from Null attributes #1516
  • Update grunt-contrib-qunit and use the bridge from there (Adjust bridge for use with AMD gruntjs/grunt-contrib-qunit#94)

@jzaefferer
Copy link
Member

Reproducing a comment from the original PR, still need to do that:

Can we smash all the files in test/unit/helper/ into one js file? I don't see the point in splitting those up when we use them on every page. And you can get rid of the window.require dependency by exporting the right variable: http://requirejs.org/docs/api.html#config

var require = { ... };

Would also remove the need to export window.helper.jqueryUrl at all, since you could just use that directly.

@scottgonzalez
Copy link
Member Author

This is ready for a review. I've made a lot of changes from the original implementation. The new implementation significantly reduces the boilerplate for each test.

@scottgonzalez
Copy link
Member Author

grunt-contrib-qunit doesn't support AMD 😢

@arschmitz
Copy link
Member

Just went through this with @scottgonzalez on skype every thing looks good.

👍

We also talked about once this lands, seeing what we can extract here including the helpers and common tests to share with mobile.

@jzaefferer
Copy link
Member

Updated master to QUnit 1.18.0.

@scottgonzalez
Copy link
Member Author

Great. I've got a few other tasks to work on right now, but I'll finish this up soon.

@scottgonzalez
Copy link
Member Author

I've squashed most of these commits and rebased on master. New PR is at #1528.

@jzaefferer jzaefferer deleted the amd-tests branch May 15, 2015 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants