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

[BUGFIX release] Ensure Engine Routes are deactivated before destruction #14237

Merged
merged 1 commit into from
Sep 10, 2016

Conversation

trentmwillis
Copy link
Member

Makes tests fail to reproduce #14234.

Causes some existing tests to fail, though I can add a totally separate tests for this if needed.

@rwjblue
Copy link
Member

rwjblue commented Sep 8, 2016

Awesome, thank you!

I can add a totally separate tests for this if needed

That would be great if you don't mind. If nothing more it helps us ensure that our tests document the regression, and help ensure we don't accidentally "remove that errant set in deactivate" in the future.

@trentmwillis
Copy link
Member Author

@rwjblue updated. A bit more clear as to what is being tested now 😄

@trentmwillis trentmwillis changed the title Add deactivate hook to Engine tests to verify proper teardown sequence Add test to verify Engine Routes are deactivated before being destroyed Sep 8, 2016
@rwjblue
Copy link
Member

rwjblue commented Sep 8, 2016

Awesome! We should probably add some general purpose lifecycle ordering tests too at some point, but this is perfect to help track down the regression (and prevent it happening again in the future).

@homu
Copy link
Contributor

homu commented Sep 9, 2016

☔ The latest upstream changes (presumably #14244) made this pull request unmergeable. Please resolve the merge conflicts.

@trentmwillis trentmwillis force-pushed the engine-deactivate branch 2 times, most recently from bb5fe60 to dd95e05 Compare September 10, 2016 01:37
@trentmwillis
Copy link
Member Author

@rwjblue updated with a fix. Tests passing except Sauce, which seems to have failed to connect properly.

What should I tag this as? BUGFIX release?

@rwjblue
Copy link
Member

rwjblue commented Sep 10, 2016

@trentmwillis - Yep, [BUGFIX release] sounds correct to me.

@trentmwillis trentmwillis changed the title Add test to verify Engine Routes are deactivated before being destroyed [BUGFIX release] Ensure Engine Routes are deactivated before destruction Sep 10, 2016
@trentmwillis
Copy link
Member Author

Tests are passing. Pushing the amended commit message.

@rwjblue rwjblue merged commit c45d219 into emberjs:master Sep 10, 2016
@rwjblue
Copy link
Member

rwjblue commented Sep 10, 2016

This has been cherry picked into beta and release branches. Please test those channels (once Travis has pushed the builds up) to confirm before final release...

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.

3 participants