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.

Sign up for GitHub

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

Jump to bottom

Support testing without an application template wrapper. #305

Merged
merged 11 commits into from
Feb 10, 2018
Merged

Support testing without an application template wrapper. #305

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b33e608
Support apps with Feature Flag ember-glimmer-remove-application-templ…
cibernox Jan 21, 2018
008a507
Apply last feedback
cibernox Feb 7, 2018
cf11a35
Use official mechanism for feature testing.
rwjblue Feb 9, 2018
6108cc3
Remove extra assertions which require wrapper application template.
rwjblue Feb 9, 2018
b2708fa
Correct yarn version (use 3.0.0-beta series).
rwjblue Feb 10, 2018
7d0b188
Fix moduleForComponent with missing template wrapper.
rwjblue Feb 10, 2018
71d0ec0
Use explicit check for checking for application template wrapper.
rwjblue Feb 10, 2018
cce1782
Cleanup linting enable/disable for EmberENV.
rwjblue Feb 10, 2018
7af8ea2
Simplify naming of without application wrapper scenario.
rwjblue Feb 10, 2018
df741fb
Test clicking "wormholed" content with and without wrapper div.
rwjblue Feb 10, 2018
a16b9d3
Ensure tests exist for where this.element goes.
rwjblue Feb 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ jobs:
- env: EMBER_TRY_SCENARIO=ember-beta
- env: EMBER_TRY_SCENARIO=ember-canary
- env: EMBER_TRY_SCENARIO=ember-default-with-jquery
- env: EMBER_TRY_SCENARIO=ember-canary-without-application-wrapper
Copy link
Member

Choose a reason for hiding this comment

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

can we drop the canary from the scenario name? I assume this functionality will eventually exist outside of canary and that currently it uses canary is only an implementation detail, right?

Copy link
Member

Choose a reason for hiding this comment

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

yep, will do


# runs deploy if running on a specific tag
- stage: deploy
Expand Down
7 changes: 6 additions & 1 deletion addon-test-support/@ember/test-helpers/setup-application-context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@ export function visit() {
return owner.visit(...arguments);
})
.then(() => {
context.element = document.querySelector('#ember-testing > .ember-view');
// eslint-disable-next-line
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer /* global EmberENV */ because eslint-disable-next-line disables all linting rules

Copy link
Member

Choose a reason for hiding this comment

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

agree, will update

if (EmberENV._APPLICATION_TEMPLATE_WRAPPER !== false) {
context.element = document.querySelector('#ember-testing > .ember-view');
} else {
context.element = document.querySelector('#ember-testing');
}
})
.then(settled);
}
Expand Down
7 changes: 6 additions & 1 deletion addon-test-support/@ember/test-helpers/setup-rendering-context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,12 @@ export default function setupRenderingContext(context) {
// In older Ember versions (2.4) the element itself is not stable,
// and therefore we cannot update the `this.element` until after the
// rendering is completed
context.element = getRootElement().querySelector('.ember-view');
// eslint-disable-next-line
Copy link
Member

Choose a reason for hiding this comment

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

see above

if (EmberENV._APPLICATION_TEMPLATE_WRAPPER !== false) {
context.element = getRootElement().querySelector('.ember-view');
} else {
context.element = getRootElement();
}

return context;
});
Expand Down
13 changes: 9 additions & 4 deletions addon-test-support/ember-test-helpers/legacy-0-6-x/test-module-for-component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* globals EmberENV */
import { set, setProperties, get, getProperties } from '@ember/object';
import $ from 'jquery';
import { isArray } from '@ember/array';
Expand Down Expand Up @@ -278,10 +279,14 @@ export function setupComponentIntegrationTest() {
hasRendered = true;
}

// ensure the element is based on the wrapping toplevel view
// Ember still wraps the main application template with a
// normal tagged view
context._element = element = document.querySelector('#ember-testing > .ember-view');
if (EmberENV._APPLICATION_TEMPLATE_WRAPPER !== false) {
// ensure the element is based on the wrapping toplevel view
// Ember still wraps the main application template with a
// normal tagged view
context._element = element = document.querySelector('#ember-testing > .ember-view');
} else {
context._element = element = document.querySelector('#ember-testing');
}
};

context.$ = function(selector) {
Expand Down
11 changes: 11 additions & 0 deletions config/ember-try.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,17 @@ module.exports = function() {
},
},
},
{
name: 'ember-canary-without-application-wrapper',
env: {
EMBER_OPTIONAL_FEATURES: JSON.stringify({ 'application-template-wrapper': false }),
},
npm: {
devDependencies: {
'ember-source': urls[2],
},
},
},
{
name: 'ember-default-with-jquery',
npm: {
Expand Down
5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"ember-cli-htmlbars-inline-precompile": "^1.0.0"
},
"devDependencies": {
"@ember/optional-features": "^0.5.1",
"documentation": "^5.3.5",
"ember-cli": "~2.18.0",
"ember-cli-dependency-checker": "^2.0.0",
Expand All @@ -54,9 +55,9 @@
"ember-maybe-import-regenerator-for-testing": "^1.0.0",
"ember-native-dom-event-dispatcher": "^0.6.3",
"ember-resolver": "^4.0.0",
"ember-source": "~2.18.0",
"ember-source": "~3.0.0-beta.5",
"ember-source-channel-url": "^1.0.1",
"ember-try": "^0.2.23",
"ember-try": "^1.0.0-beta.1",
Copy link
Member

Choose a reason for hiding this comment

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

offtopic: mmhhh.... I have thoughts about promoting this to 1.0.0...

Copy link
Member

Choose a reason for hiding this comment

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

haha, we can always make a 2.0 😸 If there are specific things wrong with ember-try, please make issues so we can discuss the details.

IMHO, there is basically no reason to not have the ability to signal "new API's are added" (normal minor version bump) from "bug fixes" (normal patch release).

Copy link
Member

Choose a reason for hiding this comment

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

ember-cli/ember-try#155

"eslint-config-prettier": "^2.6.0",
"eslint-plugin-disable-features": "^0.1.3",
"eslint-plugin-node": "^5.2.1",
Expand Down
8 changes: 0 additions & 8 deletions tests/integration/setup-rendering-context-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,18 +119,10 @@ module('setupRenderingContext "real world"', function(hooks) {
test('can click on a sibling element of this.element (within the rootElement)', async function(assert) {
let rootElement = document.getElementById('ember-testing');

assert.notEqual(
rootElement,
this.element,
'precond - confirm that the rootElement is different from this.element'
);
Copy link
Member

Choose a reason for hiding this comment

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

should we keep this assertion and run it conditionally instead?

Copy link
Member

Choose a reason for hiding this comment

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

ya, I'll make it two tests


this.set('rootElement', rootElement);

await render(hbs`{{#-in-element rootElement}}{{click-me-button}}{{/-in-element}}`);

// this will need to be modified / removed once RFC280 lands
assert.equal(this.element.textContent, '', 'no content is contained _within_ this.element');
Copy link
Member

Choose a reason for hiding this comment

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

why is this removed?

Copy link
Member

Choose a reason for hiding this comment

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

Because this.element (when the optional flag is enabled) will have content. I can swap things around a bit and guard the assertions, but I think I'd rather have two tests instead. One for "with a wrapper div" and one "without a wrapper"...

Copy link
Member

Choose a reason for hiding this comment

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

what content will be in there and why wasn't it in there before? I'm not sure I understand the assertion 🤔

Copy link
Member

Choose a reason for hiding this comment

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

The test (as it exists on master) is actually testing a few different things:

  • this.element within the test context is not the #ember-testing div
  • when rendering only content that is "wormholed" (via in-element in this case), the actual this.element is still empty (because the wormholed content is somewhere else)
  • that we can interact (e.g. click) with elements that are within the #ember-testing div (even though they are above the this.element).

As you can tell some of these assertions are no longer needed with the changes from emberjs/rfcs#280. Specifically:

this.element within the test context is not the #ember-testing div

without the wrapper DIV, the this.element actually is #ember-testing

when rendering only content that is "wormholed", the actual this.element is still empty

due to the above, this is not true any longer. this.element will have content (the wormholed content)

These two changes are the assertions that were being removed (when you commented). I have changed things around a bit so that we have two different tests (one with an application template wrapper and one without it).

Copy link
Member

Choose a reason for hiding this comment

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

that we can interact (e.g. click) with elements that are within the #ember-testing div (even though they are above the this.element).

given that we use document.querySelector('#ember-testing > .ember-view') how is click() able to interact with elements above this.element? last time I've tried it didn't work with wormholed content (yet) 🤔

Copy link
Member

@rwjblue rwjblue Feb 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This behavior was changed in #295. click and friends are always operating on the root element (#ember-testing generally speaking) which is not this.element. The specific test that we are commenting on ATM is actually introduce in that PR specifically to address the wormholed content scenario...

tldr; we don't use document.querySelector('#ember-testing > .ember-view') for interaction helpers as of that PR

Copy link
Member

Choose a reason for hiding this comment

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

I see. Turns out we are still on v0.7.10 of @ember/test-helpers (though newest ember-cli-qunit) which is the reason why it didn't work when I tried it.

assert.equal(
rootElement.textContent,
'Click Me!',
Expand Down
Loading