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

0.6.3 is breaking existing tests #203

Closed
wouterds opened this issue Apr 10, 2017 · 5 comments
Closed

0.6.3 is breaking existing tests #203

wouterds opened this issue Apr 10, 2017 · 5 comments

Comments

@wouterds
Copy link

wouterds commented Apr 10, 2017

When downgrading to 0.6.2 the tests are passing again, so something definitely changed.

Some output from the test:

not ok 162 PhantomJS 2.1 - watchEvent: Serialized object should contain asset_id and marker
    ---
        expected: >
            true
        stack: >
            http://localhost:81/assets/tests-e567361bc39c6d7989c5823c3f732526.js:1571:14
            wrapper@http://localhost:81/assets/test-support-fc81c327ee49b5d3211b0e8a07279ff6.js:7123:34
            runTest@http://localhost:81/assets/test-support-fc81c327ee49b5d3211b0e8a07279ff6.js:3111:32
            run@http://localhost:81/assets/test-support-fc81c327ee49b5d3211b0e8a07279ff6.js:3096:11
            http://localhost:81/assets/test-support-fc81c327ee49b5d3211b0e8a07279ff6.js:3238:14
            process@http://localhost:81/assets/test-support-fc81c327ee49b5d3211b0e8a07279ff6.js:2897:24
            begin@http://localhost:81/assets/test-support-fc81c327ee49b5d3211b0e8a07279ff6.js:2879:9
            http://localhost:81/assets/test-support-fc81c327ee49b5d3211b0e8a07279ff6.js:2939:9
        message: >
            failed, expected argument to be truthy, was: undefined
        Log: |
    ...
ok 163 PhantomJS 2.1 - ESLint - unit/serializers/watch-event-test.js: should pass ESLint

The test itself:

//jscs:disable requireCamelCaseOrUpperCaseIdentifiers

import { moduleForModel, test } from 'ember-qunit';

moduleForModel('watchEvent', 'serializer:watchEvent', {
  needs: ['serializer:watchEvent'],
});

test('Should serialize', function(assert) {

  var record = this.subject();

  var serializedRecord = record.serialize();

  assert.ok(serializedRecord);

});

test('Serialized object should contain asset_id and marker', function(assert) {

  var record = this.subject({
    assetId: 'XXX',
    marker: 10,
  });

  var serializedRecord = record.serialize();

  assert.ok(serializedRecord && serializedRecord.asset_id && serializedRecord.marker && serializedRecord.asset_id === 'XXX' && serializedRecord.marker === 10);

});

Cheers,
Wouter

@Turbo87
Copy link
Member

Turbo87 commented Apr 10, 2017

can you provide a repository that reproduces this issue? otherwise this will be hard/impossible to fix

@wouterds
Copy link
Author

Sadly I can not. But I will investigate a bit more this afternoon and compare versions 0.6.2 & 0.6.3 to see what could be causing the issue.

@rwjblue
Copy link
Member

rwjblue commented Apr 10, 2017

The only real change between 0.6.2 and 0.6.3 is #198 (see full diff here).

I'm not really sure why importing require instead of using the global require would break your test in this way though...

@trentmwillis
Copy link
Member

As a general note, this assertion is problematic:

assert.ok(serializedRecord && serializedRecord.asset_id && serializedRecord.marker && serializedRecord.asset_id === 'XXX' && serializedRecord.marker === 10);

As we can no longer tell which part of this is failing. It could very well be that everything is correct, except the marker, but since this is one large compound assertion, you don't get that meaningful feedback.

You should instead have separate assertions with meaningful messages:

assert.ok(serializedRecord, 'record is serialized');
assert.equal(serializedRecord.asset_id, 'XXX', 'serialized record has correct asset id');
assert.equal(serializedRecord.marker, 10, 'serialized record has correct marker');

All of that to say, I'm skeptical that ember-test-helpers caused this breakage. Refactoring the assertion to give more granular feedback will be helpful, but a reproduction would be best.

@Turbo87
Copy link
Member

Turbo87 commented Oct 14, 2017

closing due to inactivity

@Turbo87 Turbo87 closed this as completed Oct 14, 2017
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

No branches or pull requests

4 participants