Skip to content

[Glimmer2] Mark glimmer2 tests on context.isTruthy to be @htmlbars only #13441

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

Closed
wants to merge 5 commits into from
Closed

[Glimmer2] Mark glimmer2 tests on context.isTruthy to be @htmlbars only #13441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0e5ac4e
[Glimmer2] Mark glimmer2 tests on context.isTruthy to be @htmlbars only
May 2, 2016
ae749dc
Add test for setting isTruthy on object proxies
May 4, 2016
726899c
Herp derp
May 5, 2016
69e74ec
Clarifying some comments
May 5, 2016
b92fb2c
Remove setting isTruthy in with-test
May 12, 2016
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
13 changes: 2 additions & 11 deletions packages/ember-glimmer/lib/utils/references.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,9 @@ import { dasherize } from 'ember-runtime/system/string';
import { meta as metaFor } from 'ember-metal/meta';
import { watchKey } from 'ember-metal/watch_key';
import isEnabled from 'ember-metal/features';
export const UPDATE = symbol('UPDATE');
import { isProxy } from 'ember-runtime/mixins/-proxy';

// FIXME: fix tests that uses a "fake" proxy (i.e. a POJOs that "happen" to
// have an `isTruthy` property on them). This is not actually supported –
// we should fix the tests to use an actual proxy. When that's done, we should
// remove this and use the real `isProxy` from `ember-metal`.
//
// import { isProxy } from 'ember-metal/-proxy';
//
function isProxy(obj) {
return (obj && typeof obj === 'object' && 'isTruthy' in obj);
}
export const UPDATE = symbol('UPDATE');

// @implements PathReference
export class PrimitiveReference extends ConstReference {
Expand Down
13 changes: 5 additions & 8 deletions packages/ember-glimmer/tests/integration/syntax/with-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { A as emberA } from 'ember-runtime/system/native_array';
import { moduleFor, RenderingTest } from '../../utils/test-case';
import { TogglingSyntaxConditionalsTest } from '../../utils/shared-conditional-tests';
import { strip } from '../../utils/abstract-test-case';
import ObjectProxy from 'ember-runtime/system/object_proxy';

Copy link
Member

@chancancode chancancode May 10, 2016

Choose a reason for hiding this comment

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

can you change line 170 to set proxyThing.content? We should not be setting isTruthy directly (and I guess line 174 doesn't make sense anymore)

moduleFor('Syntax test: {{#with}}', class extends TogglingSyntaxConditionalsTest {

Expand Down Expand Up @@ -153,7 +154,7 @@ moduleFor('Syntax test: {{#with as}}', class extends TogglingSyntaxConditionalsT

['@test can access alias of a proxy']() {
this.render(`{{#with proxyThing as |person|}}{{person.name}}{{/with}}`, {
proxyThing: { isTruthy: true, name: 'Tom Dale' }
proxyThing: ObjectProxy.create({ content: { name: 'Tom Dale' } })
});

this.assertText('Tom Dale');
Expand All @@ -166,15 +167,11 @@ moduleFor('Syntax test: {{#with as}}', class extends TogglingSyntaxConditionalsT

this.assertText('Yehuda Katz');

this.runTask(() => set(this.context, 'proxyThing.isTruthy', false));
this.runTask(() => set(this.context, 'proxyThing.content', { name: 'Godfrey Chan' }));

this.assertText('');

this.runTask(() => set(this.context, 'proxyThing.name', 'Godfrey Chan'));

this.assertText('');
this.assertText('Godfrey Chan');

this.runTask(() => set(this.context, 'proxyThing', { isTruthy: true, name: 'Tom Dale' }));
this.runTask(() => set(this.context, 'proxyThing', ObjectProxy.create({ content: { name: 'Tom Dale' } })));

this.assertText('Tom Dale');
}
Expand Down
77 changes: 68 additions & 9 deletions packages/ember-glimmer/tests/utils/shared-conditional-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,8 @@ export class BasicConditionalsTest extends AbstractConditionalsTest {
// Testing behaviors related to objects, object proxies, `{ isTruthy: (true|false) }`, etc
export const ObjectTestCases = {

['@test it tests for `isTruthy` if available']() {
// Marking as @htmlbars since POJOs can no longer masquerade as Proxies in glimmer2. See proxy tests below.
['@htmlbars it tests for `isTruthy` if available']() {
this.renderValues({ isTruthy: this.truthyValue }, { isTruthy: this.falsyValue });

this.assertText('T1F2');
Expand Down Expand Up @@ -283,7 +284,8 @@ export const ObjectTestCases = {
this.assertText('T1F2');
},

['@test it tests for `isTruthy` on Ember objects if available']() {
// Marking as @htmlbars since POJOs can no longer masquerade as Proxies in glimmer2. See proxy tests below.
['@htmlbars it tests for `isTruthy` on Ember objects if available']() {
Copy link
Member

@rwjblue rwjblue Jun 11, 2016

Choose a reason for hiding this comment

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

If this is not supported behavior (and that is basically what we are saying), we should just remove this test.

Copy link
Contributor Author

@Joelkang Joelkang Jun 11, 2016

Choose a reason for hiding this comment

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

Is it not problematic to remove code coverage for legacy behaviour though?

this.renderValues(
EmberObject.create({ isTruthy: this.truthyValue }),
EmberObject.create({ isTruthy: this.falsyValue })
Expand Down Expand Up @@ -348,6 +350,59 @@ export const ObjectTestCases = {
set(this.context, 'cond3', ObjectProxy.create({ content: null }));
});

this.assertText('T1T2F3');
},

['@test setting the isTruthy property of an object proxy overrides its truthiness regardless of its content']() {
this.renderValues(
ObjectProxy.create({ content: {} }),
EmberObject.create({ content: false, isTruthy: true }),
Copy link
Member

@chancancode chancancode May 10, 2016

Choose a reason for hiding this comment

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

wow, EmberObject is a proxy ?________?

Copy link
Contributor Author

@Joelkang Joelkang May 11, 2016

Choose a reason for hiding this comment

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

See my comment below to @krisselden's point about this particular test case.

The htmlbars test that this was replacing seemed to be testing the truthiness of a non-proxy object that masqueraded as a proxy via the isTruthy property. I didn't know if we wanted to just remove it completely since we would theoretically be removing test coverage (although for what exactly I cannot say) from htmlbars, so I ported the spirit of the test over with these strange cases.

ObjectProxy.create({ content: null })
);

this.assertText('T1T2F3');

this.runTask(() => this.rerender());

this.assertText('T1T2F3');

this.runTask(() => {
set(this.context, 'cond1.isTruthy', false);
set(this.context, 'cond2.isTruthy', false);
Copy link
Member

@chancancode chancancode May 10, 2016

Choose a reason for hiding this comment

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

ditto – we should not be setting isTruthy on the proxy, just change/set the content

Copy link
Contributor Author

@Joelkang Joelkang May 11, 2016

Choose a reason for hiding this comment

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

Same as above

});

this.assertText('F1F2F3');

this.runTask(() => {
set(this.context, 'cond1.content', 1);
set(this.context, 'cond2.content', 1);
set(this.context, 'cond3.content', 1);
});

this.assertText('F1F2T3');


this.runTask(() => {
set(this.context, 'cond1.isTruthy', true);
Copy link
Contributor

@krisselden krisselden May 11, 2016

Choose a reason for hiding this comment

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

why are we testing setting isTruthy directly?

set(this.context, 'cond2.isTruthy', true);
});

this.assertText('T1T2T3');

this.runTask(() => {
set(this.context, 'cond1.content', null);
set(this.context, 'cond2.content', null);
set(this.context, 'cond3.content', null);
});

this.assertText('T1T2F3');

this.runTask(() => {
set(this.context, 'cond1', ObjectProxy.create({ content: {} }));
set(this.context, 'cond2', EmberObject.create({ content: true, isTruthy: true }));
set(this.context, 'cond3', ObjectProxy.create({ content: null }));
});

this.assertText('T1T2F3');
}

Expand Down Expand Up @@ -474,7 +529,7 @@ applyMixins(TogglingConditionalsTest,
{ foo: 'bar' },
EmberObject.create(),
EmberObject.create({ foo: 'bar' }),
EmberObject.create({ isTruthy: true }),
ObjectProxy.create({ content: true }),
/*jshint -W053 */
new String('hello'),
new String(''),
Expand All @@ -492,7 +547,7 @@ applyMixins(TogglingConditionalsTest,
0,
[],
emberA(),
EmberObject.create({ isTruthy: false })
ObjectProxy.create({ content: undefined })
]),

new IsTruthyGenerator([
Expand All @@ -505,11 +560,14 @@ applyMixins(TogglingConditionalsTest,
1,
['hello'],
emberA(['hello']),
ArrayProxy.create({ content: ['hello'] }),
ArrayProxy.create({ content: [] }),
{},
{ foo: 'bar' },
EmberObject.create(),
EmberObject.create({ foo: 'bar' }),
EmberObject.create({ isTruthy: true }),
ObjectProxy.create({ content: true }),
ObjectProxy.create({ content: undefined }),
/*jshint -W053 */
new String('hello'),
new String(''),
Expand All @@ -523,8 +581,7 @@ applyMixins(TogglingConditionalsTest,
'',
0,
[],
emberA(),
EmberObject.create({ isTruthy: false })
emberA()
]),

ObjectTestCases,
Expand Down Expand Up @@ -585,7 +642,8 @@ export class TogglingHelperConditionalsTest extends TogglingConditionalsTest {
this.assertText('T1F2');
}

['@test it tests for `isTruthy` on the context if available']() {
// This no longer makes sense for glimmer since `this` refers to the component, which cannot be a proxy
['@htmlbars it tests for `isTruthy` on the context if available']() {
Copy link
Member

@chancancode chancancode May 10, 2016

Choose a reason for hiding this comment

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

Do we have another shared/generic test that tests changing the content of the proxy?

Copy link
Contributor Author

@Joelkang Joelkang May 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The ArrayTestCases and ObjectTestCases do provide coverage for proxy content changing, and are mixed into the TogglingConditionalsTest.

  • with-tests.js extends TogglingSyntaxConditionalsTest which in turn is an extension of TogglingConditionalsTest.
  • each-tests.js explicitly mixes in the ArrayTestCases
  • if-unless-test.js extends TogglingHelperConditionalsTest which extends TogglingSyntaxConditionalsTest

let template = this.wrappedTemplateFor({ cond: 'this', truthy: '"T1"', falsy: '"F1"' });

this.render(template, { isTruthy: this.truthyValue });
Expand Down Expand Up @@ -715,7 +773,8 @@ export class TogglingSyntaxConditionalsTest extends TogglingConditionalsTest {
this.assertText('T1F2');
}

['@test it tests for `isTruthy` on the context if available']() {
// Marking as @htmlbars since this no longer relevant for glimmer as `this` refers to the component, which is never a proxy
['@htmlbars it tests for `isTruthy` on the context if available']() {
Copy link
Member

@chancancode chancancode May 10, 2016

Choose a reason for hiding this comment

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

ditto, do we have another shared/generic test that tests changing the content of the proxy?

let template = this.wrappedTemplateFor({ cond: 'this', truthy: 'T1', falsy: 'F1' });

this.render(template, { isTruthy: this.truthyValue });
Expand Down