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

considering cherry-picking {{link-to}} component integration tests fix into 1.12 #11825

Closed
blimmer opened this issue Jul 20, 2015 · 19 comments
Closed

Comments

@blimmer
Copy link

blimmer commented Jul 20, 2015

As originally noted in ember-test-helpers#41 there was an issue in Ember 1.12 with using the component integration tests on any component with a {{link-to}}. This issue was fixed by emberjs/#11639 and released in the Ember 1.13.x cycle.

However, many of us are still making the transition Ember 1.12.x -> Ember 1.13.x because many add-ons are still implementing the changes necessary for Glimmer compatibility (specifically, our shim for ember-easy-forms doesn't work with glimmer and they're not going to rewrite until Ember 2.0+). Without the changes provided by emberjs/#11639 in Ember 1.12, it limits our ability to stay up-to-date with ember-test-helpers and to take advantage of the (extremely) helpful component integration tests, which are now generated by default in ember-mocha and ember-qunit.

Is it possible to cherry-pick the aforementioned PR into 1.12 and issue a 1.12.2?

I realize that this is outside the realm of what we'd normally do in the Ember ecosystem, but I think because 1.12.x -> 1.13.x was a large one for many in the community, it warrants consideration. Thanks for the consideration.

/cc @rwjblue

@rwjblue
Copy link
Member

rwjblue commented Jul 20, 2015

Thanks for bringing this up, I'll ping the rest of the team to see what their thoughts are.

@stefanpenner
Copy link
Member

@rwjblue we should likely branch 1.12 and 1.13 (once 2.0 becomes stable), so its easier for community to propose backports etc.

@blimmer blimmer changed the title considering cherry-picking {{link-to}} component integration tests fix considering cherry-picking {{link-to}} component integration tests fix into 1.12 Jul 20, 2015
@rwjblue
Copy link
Member

rwjblue commented Jul 20, 2015

Yes, my plan in general is to branch when we need to maintain a tag after tagging. We really haven't had to do it yet, so that might not be obvious from the repo branches.

@rwjblue
Copy link
Member

rwjblue commented Jul 20, 2015

@blimmer - Please submit a PR with those commits against the stable-1-12 branch.

@blimmer
Copy link
Author

blimmer commented Jul 20, 2015

Now that I tried to do the cherry-pick, I'm realizing that these commits will not cherry-pick cleanly because of the routing service introduced with 1.13. Unfortunately, I don't know the guts of ember very well, but I can try to see if there's a clear way forward.

@blimmer
Copy link
Author

blimmer commented Jul 20, 2015

For others following along, here's a simple example of where tests are broken and the stacktrace:

TypeError: Cannot read property 'recognizer' of undefined
    at computeLinkViewRouteArgs (http://localhost:7357/assets/vendor.js:29632:204)
    at Descriptor.ComputedPropertyPrototype.get (http://localhost:7357/assets/vendor.js:20511:26)
    at Object.get (http://localhost:7357/assets/vendor.js:25730:19)
    at computeLinkViewLoading (http://localhost:7357/assets/vendor.js:29463:25)
    at Descriptor.ComputedPropertyPrototype.get (http://localhost:7357/assets/vendor.js:20511:26)
    at Object.get (http://localhost:7357/assets/vendor.js:25730:19)
    at computeActive (http://localhost:7357/assets/vendor.js:29715:22)
    at computeLinkViewActive (http://localhost:7357/assets/vendor.js:29419:14)
    at Descriptor.ComputedPropertyPrototype.get (http://localhost:7357/assets/vendor.js:20511:26)
    at Object.get (http://localhost:7357/assets/vendor.js:25730:19)

UPDATE: here's the same example but with the proposed fix and tests passing.

@blimmer
Copy link
Author

blimmer commented Jul 20, 2015

So it's not a cherry-picked fix, but I got this working. See #11829

blimmer added a commit to blimmer/ember.js that referenced this issue Jul 20, 2015
@tomdale
Copy link
Member

tomdale commented Jul 21, 2015

@blimmer Doing a point release for 1.12 is highly unusual and I'd prefer not to set that precedent for anything but security releases. What is keeping you on 1.12? I'd rather prioritize fixing issues in 1.13 that are preventing you from upgrading.

@blimmer
Copy link
Author

blimmer commented Jul 21, 2015

I bet on ember-easy-form about a year ago and it has been the single biggest point of pain in upgrading Ember versions since handlebars 2.0 and Ember 1.8. I've been hobbling along using a shim that I've hacked version-by-version until now.

In order to get off of easy-forms I'd need to rewrite every form in my production app, which would take about a week and I can't sell that to my boss. It seems like they are blocked by scoped helpers and they won't fix the addon until they get them. Even if they landed in 2.x, they're not going to release a version of Easy-Form to support 1.x.

ef4 showed me how to publish a fork of Ember which I'm using right now in my app.

@stefanpenner
Copy link
Member

@bcardarella out of curiosity would you accept fixes/patches/adjustments to simple-form, so @blimmer and likely many others don't stay stuck?

@bcardarella
Copy link
Contributor

@stefanpenner the easy-form issues stem around 1.10 - 1.13 and the move to HTMLBars. Specifically, there could be a interim release to help ease migrations that packaged compiled HTMLBars templates. If someone wants to do that I can merge and release.

As far as ember-easy-form 2.0 and ember-cli I am not willing to move forward until scoped helpers are in Ember #10244

@tsing80
Copy link

tsing80 commented Jul 22, 2015

+1 for merging it into 1.12 too. There are still addons (such as ember-table) that is not ported to 1.13 yet which block me upgrading to 1.13

@stefanpenner
Copy link
Member

+1 for merging it into 1.12 too.

this is unlikely to happen

which block me upgrading to 1.13

merging this is a similar fix into 1.13 is a stretch but may be possible, i suspect we could drop the hooks in, and expose this as an addon or similar. Although tricky... without a full beta cycle

@rwjblue
Copy link
Member

rwjblue commented Jul 25, 2015

@stefanpenner - This is already fixed in 1.13.

@stefanpenner
Copy link
Member

@stefanpenner - This is already fixed in 1.13.

perfect.

@mixonic
Copy link
Sponsor Member

mixonic commented Sep 9, 2015

I've opened a fix for this in ember-test-helpers: emberjs/ember-test-helpers#100

@blimmer
Copy link
Author

blimmer commented Sep 14, 2015

I upgraded ember test helpers and that doesn't resolve this problem. I think the use of the fork is necessary still.

@rwjblue
Copy link
Member

rwjblue commented Sep 14, 2015

A previously failing test was added in emberjs/ember-test-helpers#100 confirming that {{link-to}} can be rendered in a component integration test, and that test passes on 1.10, 1.11, 1.12, 1.13, 2.0, 2.1-betas, and canary.

The test does:

test('it can render a link-to', function() {
  this.render("{{link-to 'Hi' 'index'}}");
  ok(true, 'it renders without fail');
});

@blimmer - Can you submit a PR with a few more tests over there, that show what is failing? We can try to jump in and figure out what else might be needed...

@blimmer
Copy link
Author

blimmer commented Sep 17, 2015

I spent some time trying to figure out why the written test isn't failing, and I couldn't figure it out. I did, however, make a very simple project that shows it's still failing with the most current ember-qunit, which should be using the more current ember-test-helpers.

Maybe I'm missing something, but it wasn't obvious after looking for about 30 minutes.

Failing tests
Branch of code

Easy repro steps:

  1. ember new my-app
  2. change bower.json to ember 1.12.1 and ember-qunit 0.4.11 (should have ~0.5.10 ember-test-helpers)
  3. ember g route about
  4. ember g component foo-bar
  5. change foo-bar.hbs to this: {{#link-to 'about}}About{{/link-to}}`` 6.ember t`

Still fails with this message:

at http://localhost:7357/assets/test-support.js:6499: 'undefined' is not an object (evaluating 'router.router.recognizer')

blimmer added a commit to blimmer/ember.js that referenced this issue Jan 20, 2016
mixonic referenced this issue in emberjs/ember-test-helpers Apr 13, 2016
Versions of Ember prior to 1.13 require a full router booted by the app,
or no router at all. This ensures they receive no router at all instead
of the 1.13-safe one.
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

7 participants