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

[Glimmer2] [WIP] Migrate attrs lookup tests #13203

Merged
merged 1 commit into from
Apr 11, 2016

Conversation

Joelkang
Copy link
Contributor

More or less migrated verbatim as per #13127

cc @chancancode

I'm leaving out should not need to call _super in didReceiveAttrs (GH #11992) because it's not clear to me what the test is actually doing (or how it's testing against the issue). It does look like INUR should take care of the internals of this particular test case, but please advise if otherwise.

Imo, if we really want to test that _super is not required, _super should be spied on (or clobbered with assertions?) I haven't spent enough time digging into the glimmer2 code, so I'm not even sure what the _super of didReceiveAttrs would actually be.

@Joelkang
Copy link
Contributor Author

Eh, CI failed because I moved attrs-lookup-test.js to under components in the ember-glimmer package since it made sense to me there, but obviously this fails for ember-htmlbars. I think the best solution is to move the symlink into its own folder so that when we remove ember-htmlbars we don't have to change anything. @chancancode thoughts?

this.assertText('first attr');
}

['@html should be able to modify a provided attr into local state #11571 / #11559'](assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is @html meant to be @htmlbars?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@chancancode
Copy link
Member

@Joelkang thank you for working on this (and sorry about the delay!). I left some comments inline.


this.runTask(() => set(this.context, 'firstAttr', 'second attr'));

assert.equal(instance.get('first'), 'SECOND ATTR', 'component lookup uses modified local state');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was failing for me, didRecieveAttrs() wasn't getting called after the set() in the runTask() above, but I can't for the life of me figure out why. Am pushing to see if the test fails in CI too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like it's failing in CI too; @chancancode does anything look wrong here? I've stared at it for so long and it looks logically correct to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a legit bug here. I pulled this locally and placed the a currly on the outside of the component to make sure things were sane. It appears didReceiveAttrs is not actually being re-triggered.
screen shot 2016-04-07 at 10 49 32 pm

I then decided to see if this was a shadowing issue with the attr and local property being the same. The componet code changed to setting a local property called local and using it in the template like {{local}}.

let FooBarComponent = Component.extend({
  init() {
     this._super(...arguments);
     instance = this;
     this.local = null;
  },
  didReceiveAttrs() {
    this.set('local', this.getAttr('first').toUpperCase());
  }
});

This resulted in an even more bizarre output.
screen shot 2016-04-07 at 11 14 56 pm

So it started updating but then local was not uppercase. Adding @rwjblue since he may have more context since he added the original tests around this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This is indeed pretty weird, especially when it works in apps. I don't have time to investigate this right now, but I don't want to block this PR on that. Since this is new stuff we added (i.e. we are not loosing coverage), can you comment out the failing steps for now with a TODO comment pointing to this thread? We can revisit this when we enable this test for Glimmer – re-implementing it in Glimmer will probably teach us why.

LGTM otherwise!

@Joelkang
Copy link
Contributor Author

Commented out the failing test and squashed; thanks @chancancode!

@chancancode chancancode merged commit ddd0de8 into emberjs:master Apr 11, 2016
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

3 participants