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

Support testing without an application template wrapper. #305

Merged
merged 11 commits into from
Feb 10, 2018

Conversation

cibernox
Copy link
Contributor

Is this the best way to check if a feature flag is enabled?

@@ -19,7 +19,12 @@ export function visit() {
return owner.visit(...arguments);
})
.then(() => {
context.element = document.querySelector('#ember-testing > .ember-view');
// eslint-disable-next-line
if (EmberENV.FEATURES['ember-glimmer-remove-application-template-wrapper']) {
Copy link
Member

Choose a reason for hiding this comment

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

This might work for now, but isn't a good long term solution (once it moves to beta this will be falsey).

I think what we need to do is render an empty template, detect if there are any ember-views and then memoize (so we only have to do the extra rendering one time for the entire suite)/

Copy link
Member

Choose a reason for hiding this comment

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

@cibernox - Now that this has settled, we can use EmberENV._APPLICATION_TEMPLATE_WRAPPER. A true value means we need #ember-testing > .ember-view and a false value means we need #ember-testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update it.

@@ -19,7 +19,12 @@ export function visit() {
return owner.visit(...arguments);
})
.then(() => {
context.element = document.querySelector('#ember-testing > .ember-view');
// eslint-disable-next-line
if (EmberENV.FEATURES['ember-glimmer-remove-application-template-wrapper']) {
Copy link
Member

Choose a reason for hiding this comment

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

@cibernox - Now that this has settled, we can use EmberENV._APPLICATION_TEMPLATE_WRAPPER. A true value means we need #ember-testing > .ember-view and a false value means we need #ember-testing.

@@ -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
if (EmberENV.FEATURES['ember-glimmer-remove-application-template-wrapper']) {
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, should be able to use EmberENV._APPLICATION_TEMPLATE_WRAPPER here.

@@ -11,6 +11,7 @@ module.exports = function(environment) {
FEATURES: {
// Here you can enable experimental features on an ember canary build
// e.g. 'with-controller': true
'ember-glimmer-remove-application-template-wrapper': true,
Copy link
Member

Choose a reason for hiding this comment

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

Lets remove this.

In order to test (until ember-cli/ember-try#181 lands in a release) we will need to:

  1. add an ember-try scenario for ember-canary-without-application-wrapper
  2. add that scenario to .travis.yml
  3. update tests/dummy/config/environment.js to do:
EmberENV: {
  // ...snip other normal stuff...
  _APPLICATION_TEMPLATE_WRAPPER: process.env.EMBER_TRY_CURRENT_SCENARIO === 'ember-canary-without-application-wrapper' ? false : true
}

@cibernox
Copy link
Contributor Author

cibernox commented Feb 7, 2018

@rwjblue I think this is ready.
However, how some the feature flag has become an underscored prop in EmberENV. Is this FF still enabled the same way?

@rwjblue
Copy link
Member

rwjblue commented Feb 7, 2018

However, how some the feature flag has become an underscored prop in EmberENV. Is this FF still enabled the same way?

It is expected to be enabled via the @ember/optional-features addon which takes care of setting up the private environment flag. In the long run, all of this will be moved to be build time (so we don't ship both sets of implementation in Ember) but for now it is runtime (based on EmberENV._APPLICATION_TEMPLATE_WRAPPER).

I'll update this addon in a followup PR to leverage @ember/optional-features and ember-try's integration...

@cibernox
Copy link
Contributor Author

cibernox commented Feb 7, 2018

@rwjblue Do the failures make sense to you?

@rwjblue
Copy link
Member

rwjblue commented Feb 9, 2018

Yes, I am working on fixing the canary failure in emberjs/ember.js#16232.

Also, I pushed some commits which cleanup the testing harness a bit (to more of what we expect "normal" addons to do going forward).

@rwjblue
Copy link
Member

rwjblue commented Feb 10, 2018

ZOMG, finally green.

@rwjblue rwjblue requested a review from Turbo87 February 10, 2018 11:45
@rwjblue rwjblue changed the title [WIP]Support apps with Feature Flag ember-glimmer-remove-application-template-wrapper enabled Support apps with Feature Flag ember-glimmer-remove-application-template-wrapper enabled Feb 10, 2018
@cibernox
Copy link
Contributor Author

Looks great. :shipit:

.travis.yml Outdated
@@ -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

@@ -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

@@ -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

"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.

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

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.

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

@rwjblue rwjblue changed the title Support apps with Feature Flag ember-glimmer-remove-application-template-wrapper enabled Support testing without an application template wrapper. Feb 10, 2018
@rwjblue
Copy link
Member

rwjblue commented Feb 10, 2018

Created emberjs/ember-optional-features#10 to track swapping out the manual EmberENV checks in favor of something more reliable provided by @ember/optional-features.

@rwjblue rwjblue merged commit 39e8194 into emberjs:master Feb 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants