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

Update assert against nulls #5218

Merged
merged 4 commits into from
Oct 25, 2017
Merged

Update assert against nulls #5218

merged 4 commits into from
Oct 25, 2017

Conversation

pangratz
Copy link
Member

Does this make sense? Basically this is to prevent us from future :trollface:ing like reported in #5207. I am unsure if this is a good idea or if we should update the tests to use assert.ok / assert.notOk instead 🤔 . Basically just did a search and replace...

@pangratz pangratz requested a review from bmac October 10, 2017 08:26
@rwjblue
Copy link
Member

rwjblue commented Oct 10, 2017

950) [PhantomJS 2.1] integration/store/query: meta is proxied correctly on the PromiseArray

^ is failing the build on Ember 1.13 and 2.4 (but passes on 2.8+).

@pangratz
Copy link
Member Author

Good catch @rwjblue! Seems like this is failing since #5213. Any quick idea on what's wrong with that PR and earlier Ember versions?

@pangratz
Copy link
Member Author

I can't reproduce locally via ember try:one ember-1-13 --- ember test 😔

@bmac
Copy link
Member

bmac commented Oct 16, 2017

#5213 looked green on travis when I merged it...

@@ -39,7 +39,7 @@ test("meta is proxied correctly on the PromiseArray", function(assert) {
result = store.query('person', {});
});

assert.equal(result.get('meta.foo'), undefined);
assert.strictEqual(result.get('meta.foo'), undefined);
Copy link
Member

Choose a reason for hiding this comment

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

@pangratz I spent some time digging into this and couldn't figure out while its failing. I blame phantom weirdness. Lets leave this as assert.equal so the tests pass since the fact that this returns undefined is Ember behavior and not Ember Data.

Copy link
Member

Choose a reason for hiding this comment

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

Correction, I was just able to reproduce it. Turns out in Ember 1.13 Ember.get({foo: null}, 'foo.bar') returns null but in Ember 2.0 Ember.get({foo: null}, 'foo.bar') returns undefined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Uff, thanks for tracking that one down!

I've updated this assertion to be assert.notOk.

This prevents us being trolled by QUnit, where

    assert.equal(undefined, null)

is not a failing assertion.
This prevents us being trolled by QUnit, where

    assert.equal(null, undefined)

is not a failing assertion.
@bmac bmac merged commit 2e1c66f into emberjs:master Oct 25, 2017
@pangratz pangratz deleted the update-assert-against-nulls branch October 25, 2017 19:07
igorT pushed a commit that referenced this pull request Jan 29, 2018
* update assertion for meta proxied on AdapterPopulatedRecordArray

* use `assert.strictEqual` when comparing against `null`

This prevents us being trolled by QUnit, where

    assert.equal(undefined, null)

is not a failing assertion.

* use `assert.strictEqual` when comparing against `undefined`

This prevents us being trolled by QUnit, where

    assert.equal(null, undefined)

is not a failing assertion.

* align test for DateTransform#deserialize(undefined) with implementation
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