[Glimmer2] Port existing custom-helper-tests #13138

zackthehuman · 2016-03-20T02:26:46Z

This moves the custom-helper-tests into the new testing infrastructure for Glimmer 2 support (#13127). 15 of 15 tests were ported, with only a few failures for Glimmer 2.

The failing tests mostly have to do with using helpers in block form within elements:

dashed shorthand helper not usable with a block
dashed helper not usable with a block
dashed shorthand helper not usable within element
dashed helper not usable within element

However, there are a few other tests which show that helpers are being recomputed more times than necessary (at least when compared to HTMLBars).

chadhietala · 2016-03-20T02:47:00Z

packages/ember-glimmer/tests/integration/helpers/custom-helper-test.js

+    let count = 0;
+    let helper;
+
+    this.registerHelper('hello-world', {


This helper should probably take some input so that you can perform the I-N-U-R cycle described in #13127.

this.register('hello-world', { ... compute([value]) { return `${value}-value` } })

This way you can update it and reset it in the tests.

this.render(`{{hello-world foo}}`, {foo: 'hey'}); this.assertText('hey-value'); this.runTask(() => this.rerender()); this.runTask(() => set(this.context, 'foo', 'bye')); this.assertText('bye-value'); this.runTask(() => set(this.context, 'foo', 'hey')); this.assertText('hey-value');

I'll add some additional logic like you described above. There are tests specifically for no-arg helpers as well as helpers that take args.

chadhietala · 2016-03-20T02:48:21Z

Hey Zack, thanks for doing this. As I mentioned above the tests should follow the cycle.

zackthehuman · 2016-03-20T03:00:10Z

Will do. This was a straight port, so I will definitely clean it up per the instructions. I will update the title to indicate this is WIP.

zackthehuman · 2016-03-20T06:19:32Z

packages/ember-glimmer/tests/integration/helpers/custom-helper-test.js

+
+    this.assertText('bob-value');
+
+    assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');


After adding a little I-N-R-U into the mix, it appears that helpers are recomputed more often with Glimmer 2 than with HTMLBars. After a call to this.rerender() it appears to call compute() two times, based on the test failures.

@chancancode @wycats the number of invocations for helpers is purely an implementation detail, but this seems like a possible performance regression for rerender if a helper having an unchanged argument value re-computes.

I just added rerenders to a few additional places and there is definitely a correlation between rerenders and extra calls to compute.

@zackthehuman yeah, we have plans to make it work. if anything is failing because of that, feel free to skip with @htmlbars for now. I agree with @mixonic that we have some wiggle room on that, but I think we can/should make it work. Just have to circle back on that a bit later.

skip htmlbars or skip glimmer? The HTMLbars behavior is the one we want?

Or do you propose merging this with different invocation counts and opening a followup? Just want to by sure.

@htmlbars runs just htmlbars and skips glimmer

Yeah, "run it on htmlbars only", keeping the existing invocation count. We can re-visit this on Glimmer 2 and try to match the invocation count (or decide to update the tests at that point).

chancancode · 2016-03-20T18:37:51Z

By the way, the word "dashed helper" doesn't have any significance. You can just remove it from all the test names. You can still keep an dash in the helper name (or not – it doesn't behave any differently on either Glimmer or HTMLBars), that part is fine.

zackthehuman · 2016-03-20T18:53:20Z

@chancancode Yeah, I thought the wording was outdated. I had gone through and changed it to "custom helper" but then I wasn't sure if "custom" even made sense. Will update.

zackthehuman · 2016-03-21T16:39:48Z

packages/ember-glimmer/tests/integration/helpers/custom-helper-test.js

+    assert.strictEqual(destroyCount, 0, 'destroy is not called on recomputation');
+  }
+
+  ['@htmlbars helper with arg can recompute a new value']() {


This is marked as @htmlbars because Glimmer2 is causing more recomputes than HTMLBars. Rerendering causes an additional 2 computes.

Nit: I would probably call this helper with static arguments can... (and the next test with dynamic arguments can ..., to differentiate but also maintaining the parallel)

chancancode · 2016-03-21T20:15:11Z

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

-  equal(hash.last, 'sam', 'hash.last argument is sam');
-});
-
-QUnit.test('dashed helper usable in subexpressions', function() {


✔️ helper usable in subexpressions

zackthehuman · 2016-03-22T16:25:21Z

@chancancode I believe that I have addressed all of your feedback. Please take a look and see if there are any more remaining items.

chancancode · 2016-03-22T16:27:50Z

@zackthehuman thank you. Sorry I was in the middle of reviewing this yesterday and ran out of time. I believe it was mostly good to go except some consistency issues (that were in the old tests). I'll come back to this today.

chancancode · 2016-03-22T17:49:44Z

packages/ember-glimmer/tests/integration/helpers/custom-helper-test.js

@@ -22,4 +26,436 @@ moduleFor('Helpers test: custom helpers', class extends RenderingTest {
    this.assertText('hello world');
  }

+  ['@htmlbars helper can recompute a new value']() {


helper -> class-based helper

chancancode · 2016-03-22T17:55:35Z

Looks great to me! Just some minor naming consistency left (see line comments). When you have fixed those, can you also squash your commits? Thank you!

chancancode · 2016-03-24T18:59:59Z

@zackthehuman Thank you so much!

chadhietala reviewed Mar 20, 2016
View reviewed changes

zackthehuman changed the title ~~[Glimmer2] Port existing custom-helper-tests~~ [Glimmer2] WIP - Port existing custom-helper-tests Mar 20, 2016

zackthehuman reviewed Mar 20, 2016
View reviewed changes

zackthehuman changed the title ~~[Glimmer2] WIP - Port existing custom-helper-tests~~ [Glimmer2] Port existing custom-helper-tests Mar 21, 2016

zackthehuman reviewed Mar 21, 2016
View reviewed changes

chancancode reviewed Mar 21, 2016
View reviewed changes

zackthehuman force-pushed the custom-helper-test branch from 308a9d7 to 509571c Compare March 22, 2016 03:14

chancancode reviewed Mar 22, 2016
View reviewed changes

zackthehuman force-pushed the custom-helper-test branch from 509571c to 201c9e9 Compare March 23, 2016 02:23

Port custom helper tests for Glimmer 2

ef0aa64

zackthehuman force-pushed the custom-helper-test branch from 201c9e9 to ef0aa64 Compare March 23, 2016 02:27

chancancode merged commit 0569af0 into emberjs:master Mar 24, 2016

homu mentioned this pull request Mar 24, 2016

[Glimmer2] Move Helper Lookup Test #13147

Merged

chancancode mentioned this pull request Mar 28, 2016

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

Closed

45 tasks

zackthehuman deleted the custom-helper-test branch April 18, 2016 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Glimmer2] Port existing custom-helper-tests #13138

[Glimmer2] Port existing custom-helper-tests #13138

zackthehuman commented Mar 20, 2016

chadhietala Mar 20, 2016

zackthehuman Mar 20, 2016

chadhietala commented Mar 20, 2016

zackthehuman commented Mar 20, 2016

zackthehuman Mar 20, 2016

mixonic Mar 20, 2016

zackthehuman Mar 20, 2016

chancancode Mar 20, 2016

mixonic Mar 20, 2016

krisselden Mar 20, 2016

chancancode Mar 20, 2016

chancancode commented Mar 20, 2016

zackthehuman commented Mar 20, 2016

zackthehuman Mar 21, 2016

chancancode Mar 21, 2016

chancancode Mar 21, 2016

zackthehuman commented Mar 22, 2016

chancancode commented Mar 22, 2016

chancancode Mar 22, 2016

chancancode commented Mar 22, 2016

chancancode commented Mar 24, 2016


		this.assertText('bob-value');

		assert.strictEqual(computeCount, 1, 'compute is called exactly 1 time');

[Glimmer2] Port existing custom-helper-tests #13138

[Glimmer2] Port existing custom-helper-tests #13138

Conversation

zackthehuman commented Mar 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadhietala commented Mar 20, 2016

zackthehuman commented Mar 20, 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

chancancode commented Mar 20, 2016

zackthehuman commented Mar 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackthehuman commented Mar 22, 2016

chancancode commented Mar 22, 2016

Choose a reason for hiding this comment

chancancode commented Mar 22, 2016

chancancode commented Mar 24, 2016