[Glimmer2] More with tests #12927

chadhietala · 2016-02-08T02:19:21Z

Migrates {{with}} block statement tests. Should I be deleting the old HTMLBar's tests as migrate them since I'm inheriting from SharedConditionalsTest?

rwjblue · 2016-02-08T02:27:13Z

@chadhietala - Yes, deleting the old tests is correct (assuming they are being ran in both glimmer and htmlbars packages due to the symlinking).

chancancode · 2016-02-08T02:31:41Z

packages/ember-glimmer/tests/integration/syntax/with-test.js

@@ -8,10 +8,10 @@ import {

 moduleFor('Syntax test: {{#with}}', class extends SharedConditionalsTest {
  templateFor({ cond, truthy, falsy }) {
-    return `{{#with ${cond} as |test|}}${truthy}{{else}}${falsy}{{/with}}`;
+    return `{{#with ${cond}}}${truthy}{{else}}${falsy}{{/with}}`;
  }


We can quite easily subclass twice to test both versions now if we want

chadhietala · 2016-02-08T05:41:40Z

So I wasn't sure about the tests in that same file that test Handlebars and things like view and controller context, but everything else I believe is migrated.

chancancode · 2016-02-08T19:52:37Z

packages/ember-glimmer/tests/integration/syntax/with-test.js

+
+    this.runTask(() => this.rerender());
+
+    this.assertText('Stef-Yehuda-Stef');


We should go through the mutation/replacement cycle for these too

The shared conditional tests has more examples of how to generalize that pattern (I believe all of them follow the pattern)

I can repeat the render -> rerender -> mutate -> render pattern, not seeing the point of the testing pattern unless I can associate a case with a template. All of these tests are testing things like scoping.

Yeah, I can explain it. So for this particular test, I would have written:

['@test the scoped variable is not available outside the {{with}} block.']() { this.render(`{{name}}-{{#with other as |name|}}{{name}}{{/with}}-{{name}}`, { name: 'Stef', other: 'Yehuda' }); // (1) this.assertText('Stef-Yehuda-Stef'); this.runTask(() => this.rerender()); // (2) this.assertText('Stef-Yehuda-Stef'); this.runTask(() => set(this.context, other, 'Chad')); // (3) this.assertText('Stef-Chad-Stef'); this.runTask(() => set(this.context, name, 'Tom')); // (4) this.assertText('Tom-Chad-Tom'); this.runTask(() => { set(this.context, name, 'Stef'); set(this.context, other, 'Yehuda'); }); // (5) this.assertText('Stef-Yehuda-Stef'); }

Initial render

No-op rerender: the point is to test that "stable rerender" works (i.e. if nothing changes, we don't accidentally blow away the nodes. It could happen if we cached the wrong value, forgot to capture certain values, etc. Re-render is just a completely different path than initial render.)

Mutation

Mutation: could have grouped this with the above, too. I don't have a very strong preference for this. You could have as many of these as you need.

Reset: this has two purposes. Usually this is done via replacement (e.g. set(this.context, 'admin', { name: 'Tom Dale' })), but in this case, strings are immutable and there is no difference between mutation/replacement. But there is another reason – we had cases where we cached the lastValue in initial render and never updated it in re-render, so updating it to anything other than the original value would work, but you can never restore it to its original value.

In this case, splitting (3) and (4) probably makes sense – you could imagine a bug in the scoping code where changing one would accidentally overwrite the other.

As you can see, it's pretty tricky to come up with the scenarios and get the correct amount of coverage, which is why I prefer coming up with the pattern once and consistently do it everywhere.

@chancancode I think we're both confused, completely agree with the pattern you described. I should just asked for clarification in terms of the actual test plan pattern. What I mean by this is that the SharedConditionalTests have the templateFor/generator pattern. I thought you were asking to follow that setup for each moduleFor in this file.

chancancode · 2016-02-08T19:54:34Z

If the tests clearly doesn't apply anymore we can keep them in the old file. If they should apply but doesn't work on glimmer yet they can be ported but marked as @htmlbars instead of @test instead.

chadhietala · 2016-02-10T19:18:06Z

I think Travis might need to be kicked.

chancancode · 2016-02-10T21:23:02Z

packages/ember-glimmer/tests/integration/syntax/with-test.js

+
+    this.assertText('Hello');
+
+    this.runTask(() => set(this.context, 'cond1', { greeting: 'Hola' }));


Should be set(this.context, 'cond1.greeting', 'Hola') to test mutation

chancancode · 2016-02-11T21:04:48Z

packages/ember-htmlbars/tests/helpers/with_test.js

 import compile from 'ember-template-compiler/system/compile';
 import { runAppend, runDestroy } from 'ember-runtime/tests/utils';

 var view, lookup;
 var originalLookup = Ember.lookup;

-function testWithAs(moduleName, templateString, deprecated) {


Note for self: templateString is always {{#with person as |tom|}}{{title}}: {{tom.name}}{{/with}}

chancancode · 2016-02-11T21:17:13Z

@chadhietala can you do me a favor and annotate the deleted test case with comments like these? #12914 (comment) We are trying to be careful to not lose coverage as we migrate these, and having those annotations would make it a lot easier to review! ❤️

The tests doesn't always have to be a direct port. We implicitly test a lot more things with the new harness/pattern, so it is quite possible we absorbed some existing test cases into other test cases implicitly (e.g. we used to have separate test cases for re-render, but we now test re-render all the time, so it makes sense to just delete it).

chadhietala · 2016-02-12T17:11:28Z

packages/ember-htmlbars/tests/helpers/with_test.js

-  equal(view.$().text(), 'Admin: Tom Dale User: Yehuda Katz', 'should be properly scoped');
-});
-
-QUnit.test('should respect `isTruthy` field on a view', function() {


Replaced with: BASIC_TRUTHY_TESTS

Actually (correct me if I am wrong), I think we removed coverage for this – this test is about isTruthy beging set on the self context directly. I don't know why you would want to do it, and I doubt that it is super important, but it is also not hard to do it generically, so we should probably maintain it:

export class SharedConditionalsTest extends AbstractConditionalsTest { ... ['@test it tests for `isTruthy` on the context if available']() { let template = this.templateFor({ cond: 'this', truthy: 'T1', falsy: 'F1' }); this.render(template, { isTruthy: true }); this.assertText('T1'); this.runTask(() => this.rerender()); this.assertText('T1'); this.runTask(() => set(this.context, 'isTruthy', false)); this.assertText('F1'); this.runTask(() => set(this.context, 'isTruthy', true)); this.assertText('T1'); } ... }

doing ^ that in the shared case would make it available to #if,#unless,etc instead of randomly testing it for #with but nothing else

✔️ : Done

chadhietala · 2016-02-12T17:12:50Z

packages/ember-htmlbars/tests/helpers/with_test.js

-  equal(view.$().text(), 'Stef-Yehuda-Stef', 'should be properly scoped after updating');
-});
-
-QUnit.test('nested {{with}} blocks shadow the outer scoped variable properly.', function() {


Direct port.

👌 "nested {{with}} blocks shadow the outer scoped variable properly"

chancancode · 2016-02-12T20:30:04Z

packages/ember-glimmer/tests/integration/syntax/with-test.js

+    });
+
+    this.assertText('Señor Engineer: Tom Dale');
+  }


This is basically it should support #with-as syntax + updating the context should update the alias now

This also tests "#with without inverse" implicitly – which the templateFor suite does not cover

(Similar to https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/tests/integration/syntax/if-unless-test.js#L15-L41)

chancancode · 2016-02-13T01:21:52Z

packages/ember-htmlbars/tests/helpers/with_test.js

-    });
-
-    equal(view.$().text(), 'Señor Engineer: Yehuda Katz', 'should be properly scoped after updating');
-  });


folded into "can access alias and original scope"

chancancode · 2016-02-16T20:04:02Z

packages/ember-htmlbars/tests/helpers/with_test.js

-
-  runAppend(view);
-  equal(view.$().text(), 'Admin: Tom Dale User: Yehuda Katz', 'should be properly scoped');
-});


Direct port: re-using the same variable with different #with blocks does not override each other

👌

[Glimmer2] More with tests

chancancode reviewed Feb 8, 2016
View reviewed changes

chadhietala force-pushed the moar-glim-glam-with branch 3 times, most recently from c9f1144 to 3270e3f Compare February 8, 2016 05:40

chancancode reviewed Feb 8, 2016
View reviewed changes

chadhietala force-pushed the moar-glim-glam-with branch from 3270e3f to dde5cf9 Compare February 10, 2016 19:09

chancancode reviewed Feb 10, 2016
View reviewed changes

chadhietala force-pushed the moar-glim-glam-with branch 2 times, most recently from c492cdd to 3e224fc Compare February 11, 2016 15:27

chancancode reviewed Feb 11, 2016
View reviewed changes

chancancode mentioned this pull request Feb 11, 2016

[Glimmer2] inline if #12920

Merged

15 tasks

chadhietala reviewed Feb 12, 2016
View reviewed changes

chadhietala force-pushed the moar-glim-glam-with branch from 3e224fc to f9ef01b Compare February 12, 2016 17:29

chancancode reviewed Feb 12, 2016
View reviewed changes

chadhietala force-pushed the moar-glim-glam-with branch from f9ef01b to 6520c1e Compare February 12, 2016 21:57

chancancode reviewed Feb 13, 2016
View reviewed changes

chadhietala force-pushed the moar-glim-glam-with branch from 6520c1e to dc67695 Compare February 13, 2016 04:24

chancancode reviewed Feb 16, 2016
View reviewed changes

[Glimmer2] with tests

21ab6c0

chadhietala force-pushed the moar-glim-glam-with branch from dc67695 to 21ab6c0 Compare February 16, 2016 20:53

chancancode added a commit that referenced this pull request Feb 16, 2016

Merge pull request #12927 from chadhietala/moar-glim-glam-with

c609f7d

[Glimmer2] More with tests

chancancode merged commit c609f7d into emberjs:master Feb 16, 2016

chadhietala deleted the moar-glim-glam-with branch February 16, 2016 23:32

chadhietala mentioned this pull request Feb 16, 2016

Glimmer Big-Picture Integration Checklist #12907

Closed

52 tasks

chancancode mentioned this pull request Mar 19, 2016

[Pls send halp!] The Ultimate Glimmer 2 Test Porting Guide #13127

Closed

45 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Glimmer2] More with tests #12927

[Glimmer2] More with tests #12927

chadhietala commented Feb 8, 2016 •

edited

Loading

rwjblue commented Feb 8, 2016

chancancode Feb 8, 2016

chadhietala commented Feb 8, 2016

chancancode Feb 8, 2016

chancancode Feb 8, 2016

chadhietala Feb 9, 2016

chancancode Feb 9, 2016

chancancode Feb 9, 2016

chancancode Feb 9, 2016

chadhietala Feb 10, 2016

chancancode Feb 10, 2016

chancancode commented Feb 8, 2016

chadhietala commented Feb 10, 2016

chancancode Feb 10, 2016

chancancode Feb 11, 2016

chancancode commented Feb 11, 2016

chadhietala Feb 12, 2016

chancancode Feb 13, 2016

chancancode Feb 13, 2016

chancancode Feb 13, 2016

chadhietala Feb 13, 2016

chadhietala Feb 12, 2016

chancancode Feb 13, 2016

chancancode Feb 12, 2016

chancancode Feb 12, 2016

chancancode Feb 12, 2016

chancancode Feb 13, 2016

chancancode Feb 16, 2016


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

		this.assertText('Stef-Yehuda-Stef');


		this.assertText('Hello');

		this.runTask(() => set(this.context, 'cond1', { greeting: 'Hola' }));

[Glimmer2] More with tests #12927

[Glimmer2] More with tests #12927

Conversation

chadhietala commented Feb 8, 2016 • edited Loading

rwjblue commented Feb 8, 2016

Choose a reason for hiding this comment

chadhietala commented Feb 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chancancode commented Feb 8, 2016

chadhietala commented Feb 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chancancode commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadhietala commented Feb 8, 2016 •

edited

Loading