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

Extract `appendView`, `destroyView` for testing #9769

Merged
merged 1 commit into from Dec 2, 2014

Conversation

Projects
None yet
4 participants
@seanpdoyle
Contributor

seanpdoyle commented Dec 1, 2014

Defining appendView and destroyView in multiple tests is verbose,
repetitious, and copy-paste-error prone.

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Dec 1, 2014

If this type of cleanup is valuable, I'll continue the find-and-replace (there are a ton more files left). I wanted to validate the value of this type of change before going down the silver-searcher rabbit hole.

@mmun

This comment has been minimized.

Member

mmun commented Dec 1, 2014

👍

1 similar comment
@twokul

This comment has been minimized.

Member

twokul commented Dec 1, 2014

👍

@rwjblue

This comment has been minimized.

Member

rwjblue commented Dec 1, 2014

Awesome! The ember-testing package is for user app testing (not internal utilities), can you move the helper to packages/ember-views/tests/append_view_helper.js, then import from there?

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Dec 1, 2014

@rwjblue how would we import from a file in ember-views/tests? does the resolver know about files outside of lib?

Does the following work?

import { appendView, destroyView } from "ember-views/tests/view_helpers";

or do I have to import with relative paths?

Edit:

That seems to work

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:view-helpers-extraction branch 2 times, most recently Dec 1, 2014

Extract `appendView`, `destroyView` for testing
Defining `appendView` and `destroyView` in multiple tests is verbose,
repetitious, and copy-paste-error prone.

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:view-helpers-extraction branch to f1b25ab Dec 1, 2014

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Dec 1, 2014

@rwjblue I think this is ready for another look

if (view) {
run(view, 'destroy');
}
destroyView(container);

This comment has been minimized.

@rwjblue

rwjblue Dec 2, 2014

Member

I'm OK with it (makes sense what it is doing), but is somewhat confusing to read (because we aren't destroying a view). Maybe we can make the destroy helper more general (as in not in ember-view package)? Or just alias the import here as import { destroyView as destroy } from ".....";. What do you think?

This comment has been minimized.

@seanpdoyle

seanpdoyle Dec 2, 2014

Contributor

I'm ok with making it a more general wrapper around a call to .destroy().

Where should this live?

This comment has been minimized.

@rwjblue

rwjblue Dec 2, 2014

Member

Seems like it might live in ember-runtime/tests/utils or something. Its basically just the same implementation that you have here though.

This comment has been minimized.

@seanpdoyle

seanpdoyle Dec 2, 2014

Contributor

@rwjblue how do you feel about renaming the exported functions?

Their value comes from the fact that they're wrapped in a run block.

appendView -> runAppend? appendInRun?
destroyView -> runDestroy? destroyInRun?

@@ -105,7 +102,7 @@ QUnit.module("ember-htmlbars: {{bind}} with a container, block forms", {
container.optionsForType('template', { instantiate: false });
},
teardown: function() {
Ember.run(function(){
run(function(){
if (container) {
container.destroy();

This comment has been minimized.

@rwjblue

rwjblue Dec 2, 2014

Member

This can use the destroy helper like above.

This comment has been minimized.

@seanpdoyle

seanpdoyle Dec 2, 2014

Contributor

@rwjblue would two underlying calls to run conflict? could a run call occur within a run call?

This comment has been minimized.

@rwjblue

rwjblue Dec 2, 2014

Member

Yes, you can have nested run's. But it doesn't seem as though the nesting is actually needed just something like:

destroy(container);
destroy(view);
container = view = null; // unless this is done in `destroy` helper
@@ -12,6 +12,7 @@ import { set } from 'ember-metal/property_set';
import { fmt } from 'ember-runtime/system/string';
import { typeOf } from 'ember-metal/utils';
import { forEach } from 'ember-metal/enumerable_utils';
import { appendView } from "ember-views/tests/view_helpers";

This comment has been minimized.

@rwjblue

rwjblue Dec 2, 2014

Member

destroyView could be used in this file (the teardown is doing the destroy song and dance).

@rwjblue

This comment has been minimized.

Member

rwjblue commented Dec 2, 2014

Awesome, thanks for tackling this cleanup effort! The other changes/tweaks can be done in a second phase (otherwise would get too big to review).

rwjblue added a commit that referenced this pull request Dec 2, 2014

Merge pull request #9769 from seanpdoyle/view-helpers-extraction
Extract `appendView`, `destroyView` for testing

@rwjblue rwjblue merged commit 7509d9c into emberjs:master Dec 2, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

seanpdoyle added a commit to seanpdoyle/ember.js that referenced this pull request Dec 2, 2014

Finish Refactoring View Tests for HTMLBars
emberjs#9769 (comment)

Standardize and Replace calls to `append` and `destroy` with helpers
wrapped in `run` calls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment