-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Container / Registry Split + Routing Refactor causes Global App Integration Test Issues #10310
Comments
I think the problem here is that we should be resetting the app's container and registry in So we could either fix this with a quick patch to canary or wait to merge the |
@rwjblue yes, in that branch the instance is destroyed on |
The previously failing JSBin now passes properly. Thanks y'all for the hard work here! |
@rwjblue I'm experiencing this issue with Ember 1.11 Beta 5.
I found this issue and I looking into how diff --git a/vendor/ember/development/ember.js b/vendor/ember/development/ember.js
index d4778f6..9b21660 100644
--- a/vendor/ember/development/ember.js
+++ b/vendor/ember/development/ember.js
@@ -3949,7 +3949,7 @@ enifed('ember-application/system/application', ['exports', 'dag-map', 'container
return ApplicationInstance['default'].create({
customEvents: property_get.get(this, 'customEvents'),
rootElement: property_get.get(this, 'rootElement'),
- applicationRegistry: this.registry
+ applicationRegistry: this.buildRegistry()
});
}, And it fixed my problem. Before the fix above, it seemed that the Registry stayed the same, even after an |
I can confirm I'm running into this issue where |
I've also hit this on both |
I'll try to look into this today. |
we're having this issue too. @mutewinter's fix works in our case. anyone still looking at this? |
I'm seeing this on 1.11.1 too on an ember-rails app too. |
I'm seeing this same problem too when testing using 1.11, @mutewinter's fix worked for me as well. I've been banging my head on this problem for days, glad I tried the fix. |
Seeing this on 1.11.1 too and @mutewinter's fix works. This seems to be annoying and a main barrier for us to transfer to use 1.11.1 against 1.10, since almost all integration tests are broken. |
This does appear to be a current issue, can we get this resolved please! @tomdale? |
@rwjblue would this get more love if put in a current milestone (instead of the already past 1.11.0 one)? |
A good portion of our functional tests are broken because of this (under 1.11.3). @mutewinter's fix totally works. I imagine this is presently blocking a whole lot of people from moving to 1.11. When can we expect a patch release? |
Totally screwed up all integration tests for my project, once I've migrated to 1.11 and there's no way back, because I've refactored the code base to utilize new features (e.g. component helper). I'll wait patiently for the update. |
@szalishchuk In the meantime, you can fork our fork: I took the last commit for the 1.11.3 version and applied mutewinter's fix on top of that. You will need to build/deploy that Ember version, for which end you can just use |
@balinterdi Thank you very much! Why don't you submit your fix as a PR? |
The fix is not mine but @mutewinter's (see above) and since I'm not intimate with the internals of the container/registry, I'm not sure if the fix is a real solution or just a hack. Above that, a failing unit test should be provided that highlights the bug (that the fix then squashes) and I would be at a loss regarding how to write it :) |
Yeah I'm not sure if it's a hack or a real fix either. I was asked in #10597 to write a test, but I'm not sure what a test for this looks like. |
If you don't want to futz with ember itself, you can try to apply the @mutewinter fix in your test code. Wherever you do a refresh rebuild the registry first:
We made this a method called |
Throwing my hat into the ring for hacks that don't require moving from upstream: Use an initializer. Yeah. It's really, really, really horrible. :) import Ember from 'ember';
var originalBuildInstance;
export function initialize(/*container, application*/) {
originalBuildInstance = originalBuildInstance || Ember.Application.prototype.buildInstance;
Ember.Application.prototype.buildInstance = function() {
this.registry = this.buildRegistry();
return originalBuildInstance.apply(this);
};
}
export default {
name: 'horrible-reset-hack',
initialize: initialize
}; |
see emberjs/ember.js#10310 (comment) for details
@kagemusha what kind of tests are failing in your application with this fix? For me only the first test is working and after that it looks like the setup isn't creating a new ember app. |
@alvinvogelzang get the same error as without it: Cannot re-register: ..., as it has already been resolved. |
We are experiencing the same issue on version 1.13 and it's causing all of our acceptance tests to fail. We are using @ehntoo hack to temporarily run our tests, but feedback would be appreciated. |
Seeing the same issue with 1.13. |
Things seem to work properly when using both Ember and Ember Data at 2.0.0 JSBin. Using Ember and Ember Data 1.13.9 throws the error that @mutewinter mentioned above (JSBin):
|
emberjs/data#3690 should fix the issue up in Ember Data, but I am unsure if they will want to release a 1.13.12 for just that change. A potential interim work around would be adding an initializer like the following: App.initializer({
name: 'clear-service-store',
before: ['ember-data'],
initialize(registry) {
registry.unregister('service:store');
}
}); Demo: http://emberjs.jsbin.com/dawebo/1/edit?html,js,output. |
Closing this again, as I believe the work around and PR to Ember Data should solve the issue for most everyone. Sorry for the long delay on this, please let me know if there are additional errors/issues and I will reopen (if related). |
Changes to application boot on canary seem to have broken the ability to call
app.reset()
in ateardown
callback to prepare for the next test. This likely affects globals style apps more than ember-cli applications, due to the fact that a newEmber.Application
instance is created for each integration test.The issue is that when routing is started (which happens on the first
visit
after callingapp.reset()
) we attempt to registerview:toplevel
andview:default
(see here). However, due to the registry and container split, the registry is persistent acrossapp.reset()
calls, making the second call tocontainer._registry.register('view:toplevel', EmberView.extend())
to fail with the following error:Demos:
Fails with canary builds JSBin
Works with beta builds JSBin
@dgeb / @wycats / @tomdale - This is directly related to y'all's work in this area, I'd definitely appreciate your feedback and suggested path forward here.
The text was updated successfully, but these errors were encountered: