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

Use test helpers from ember dev #4040

Merged
merged 4 commits into from
Jan 14, 2016
Merged

Use test helpers from ember dev #4040

merged 4 commits into from
Jan 14, 2016

Conversation

pangratz
Copy link
Member

@pangratz pangratz commented Jan 4, 2016

This PR replaces the currently used test helpers to check for correct assertions, deprecations etcetera with the ones from emberjs/ember-dev.

This addresses #2456


TODO

  • investigate why tests are not failing although ember-dev helper raises error
  • fix failing test by wrapping this line in Ember.run
  • decide if we want to use patched QUnit.module or our own module

@pangratz pangratz changed the title Use test helper from ember dev Use test helpers from ember dev Jan 4, 2016
@pangratz
Copy link
Member Author

pangratz commented Jan 4, 2016

Hmm, looks like although the ember-dev helpers throw an error that one test has a pending run loop at the end, the build does not fail: https://travis-ci.org/pangratz/data/builds/100128108#L1023-L1032.

I am investigating...

@stefanpenner
Copy link
Member

awesome

@pangratz
Copy link
Member Author

Alright, so ember-dev is now used correctly. The failing test is expected since a run-loop is triggered here, but it is not wrapped within Ember.run.

The problem with the previous approach was that the ember-dev helpers were instantiated within QUnit.testStart() and tested in QUnit.testDone(). If an failing assertion is added within those callbacks, the corresponding test is not marked as failed.


The current, working approach patches the QUnit.module so the ember-dev helpers are setup and used in the beforeEach and afterEach hooks.

The question is now if we want to use the approach of a patched QUnit.module - like it is also done in ember-dev's setupQUnit - or if we want to use another approach, where we create a custom module and use this in all the tests:

// ember-data/tests/helpers/qunit.js
import { module } from "ember-qunit";

export default function(description, options = {}) {
  var originalBeforeEach = options.beforeEach || function() {};
  var originalAfterEach = options.afterEach || function() {};

  // ...

  return module(...arguments);
}

And use this module in all the tests (instead of import { module } from "ember-qunit";):

// ember-data/tests/integration/le-test.js
import module from "../helpers/qunit";
import { test } from "ember-qunit";

module("le module");

test("le test", function() {
  // ...
});

This change re-uses the test helpers from ember-dev to check for
expected assertions and deprecations as well as to confirm that there is
no pending run-loop at the end of a test.
Currently `this._hasModelFor` returns the specific model, though - as
the name implies - a boolean should be returned.

---

Context: this change is especially needed so the assertion helper from
ember-dev works correctly: this helper uses the checkTest function in
test-helper/utils.js https://git.io/vuZK3. If a class is passed to the
assertion, like in `Ember.assert("should exists", MyEmberClass)`, it is
treated as a function and invoked within `checkTest`. This throws an
error since ember classes are not function which can be invoked.
Getting an async`hasMany` relationship triggers a run-loop, so this
needs to be wrapped inside `Ember.run`.
@pangratz
Copy link
Member Author

I think it is ok to use the patched QUnit module version for now, as this is also the way it is done in ember-dev. Once this lands in ember-data, I will backport stuff to ember-dev so it is usable for other addons as well...

bmac added a commit that referenced this pull request Jan 14, 2016
Use test helpers from ember dev
@bmac bmac merged commit 1e5831b into emberjs:master Jan 14, 2016
@bmac
Copy link
Member

bmac commented Jan 14, 2016

Thanks @pangratz

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.

None yet

4 participants