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

FactoryFor does not trigger didReceiveAttrs hook anymore #17534

Closed
mades-g opened this issue Jan 30, 2019 · 8 comments
Closed

FactoryFor does not trigger didReceiveAttrs hook anymore #17534

mades-g opened this issue Jan 30, 2019 · 8 comments

Comments

@mades-g
Copy link

mades-g commented Jan 30, 2019

Same test, different emberjs versions, on [email protected] this "tests" would pass but recently upgraded to [email protected] and now the "test" is failing.

Previously [email protected]
Currently [email protected]

[email protected]
[email protected]

e.g test module

import { module, test } from 'qunit';
import { setupTest } from 'ember-qunit';

module('Unit | Component | foo', function(hooks) {
  setupTest(hooks);

  hooks.beforeEach(function() {
    this.componentFactory = this.owner.factoryFor('component:foo');
  });

  test('it throws', function(assert) {
    assert.expect(1);

    assert.throws(
      () => {
        this.componentFactory.create({
          fooBar: null
        });
      },
      /fooBar should not be empty/
    );
  });
});
...
didReceiveAttrs() {
    this._super(...arguments);

   assert('fooBar should not be empty', isPresent(this.get('fooBar')));
}

Same test different emberjs versions, on [email protected] this "tests" would pass but recently upgrade to [email protected] and now the "test" is failing.

The solution was to trigger the desirable hook (didReceiveAttrs) with in assert.throw:

e.g

...
  test('it throws', function(assert) {
    assert.expect(1);

    assert.throws(
      () => {
        this.componentFactory.create({
          fooBar: null
        }).trigger('didReceiveAttrs');
      },
      /fooBar should not be empty/
    );
  });
@pixelhandler
Copy link
Contributor

@mades-g thanks for the code examples, the next step will be create a working reproduction of the issue, perhaps we can make with with ember-twiddle.com

@pixelhandler
Copy link
Contributor

@mades-g I don't think this is a bug in Ember, however I think the way you test for the failure may need to change. I'll post an attempt to test it.

@pixelhandler
Copy link
Contributor

@mades-g See this test https://github.com/ember-triage/issues-17534/blob/master/tests/unit/components/x-foo-test.js#L7-L17

test('didReceiveAttrs throws', function(assert) {
  let component = this.owner.factoryFor('component:x-foo').create();
  assert.ok(component);

  assert.throws(
    () => {
      component.didReceiveAttrs();
    },
    /fooBar should not be empty/
  );
});

@mades-g
Copy link
Author

mades-g commented Feb 1, 2019

@pixelhandler Thanks for the feed back, the test type for this specific issue, is unit, and you are doing an integration test, and that's probably leveraging from setupRenderingTest and creates the right connect for the assertion.
Maybe my test should be changed to an integration, I was more concerned why it was passing before and due to recent ember-source upgrade now it's failing, but no worries on that, I'm quite new to ember so do mind my mistakes and let me know.

@mades-g
Copy link
Author

mades-g commented Feb 1, 2019

Edit: Just saw your comment comment, I know what you did works, but I wonder why it was passing in 3.1.2 🤔

@rwjblue
Copy link
Member

rwjblue commented Feb 3, 2019

didReceiveAttrs used to be called from inside the constructor, and now it isn’t. The change was done intentionally (and helped with other overall objectives), so I doubt we’d want to revert...

In general it’s best to test rendering related things by actually rendering (e.g. setupRenderingTest).

@mades-g
Copy link
Author

mades-g commented Feb 4, 2019

Cheers for the clarification everyone.
I will close this.

@mades-g mades-g closed this as completed Feb 5, 2019
@ryanone
Copy link

ryanone commented Feb 23, 2019

I ran into this issue too, and was wondering what's going on. Thanks for clarifying @rwjblue. I saw 2cd6e2f#diff-7536fca9f7b528326424427194fd9ac2 explains the rationale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants