[Glimmer2] Move escape tests to `ember-glimmer` #13143

code0100fun · 2016-03-20T23:12:49Z

Moved to content tests.

Convert capitalized path deprecation test
Convert null object properties test
Add trusted curlies moduleFor (works with TextNode ignore in assertInvariants)
HTML content in non-escaped curlies
HTML content in escaped curlies
Test all escapes of matrix values

code0100fun · 2016-03-20T23:18:05Z

packages/ember-glimmer/tests/integration/content-test.js

@@ -73,6 +73,8 @@ class DynamicContentTest extends RenderingTest {
  ['@test it can render a dynamic path']() {
    this.renderPath('message', { message: 'hello' });

+    expectNoDeprecation();


To cover this test. Not sure if this is necessary anymore. Looks like its the last piece related to deprecated global lookup (86fa0ab)?

Actually, the that test is testing that when you do {{CaptializedPath}}, and that you have a CaptializedPath property on the object, it does not emit a deprecation warning. Can you add this test back? (And remove this expectNoDeprecation)

code0100fun · 2016-03-21T00:00:05Z

packages/ember-htmlbars/tests/integration/escape_integration_test.js

-  equal(view.$().text(), '2');
-});
-
-QUnit.test('should read from an Object.create(null)', function() {


Not sure if this is now covered by these changes from c655638, @chancancode?

I think that test is slightly different: the test I added was for {{nullObject}} (which apparently does not work in htmlbars :P), whereas this is testing {{nullObject.foo}} works. So it seems fine to keep both!

chancancode · 2016-03-24T19:05:33Z

packages/ember-htmlbars/tests/integration/escape_integration_test.js

-  }
-});
-
-QUnit.test('should read from a global-ish simple local path without deprecation', function() {


https://github.com/emberjs/ember.js/pull/13143/files#r57370979

chancancode · 2016-03-24T19:15:23Z

I think most of these tests can be "matrix tested" by adding a new module:

moduleFor('Dynamic content tests ("trusted" curlies)', class extends DynamicContentTest {

  renderPath(path, context = {}) {
    this.render(`{{{${path}}}`, context);
  }

});

This would run all the shared tests cases against {{{foo}}}, following the I-N-U-R pattern as appropriate. You can then add more methods in this class to test extra stuff not covered by the shared cases.

chancancode · 2016-03-24T19:18:30Z

packages/ember-glimmer/tests/integration/content-test.js

@@ -59,6 +59,102 @@ moduleFor('Static content tests', class extends RenderingTest {

 });

+moduleFor('Null object tests', class extends RenderingTest {
+  ['@test it should read from an Object.create(null)']() {


This could probably go into the shared DynamicContentTest to test against all the possible positions. Something like:

['@test it can read from a null object']() { let nullObject = Object.create(null); nullObject['message'] = 'hello'; this.renderPath('nullObject.message', { nullObject }); this.assertText('hello'); this.assertStableRerender(); this.runTask(() => set(nullObject, 'message', 'goodbye')); this.assertText('goodbye'); this.assertInvariants(); nullObject = Object.create(null); nullObject['message'] = 'hello'; this.runTask(() => set(this.context, 'nullObject', nullObject)); this.assertText('hello'); this.assertInvariants(); }

homu · 2016-03-27T19:05:59Z

☔ The latest upstream changes (presumably #13190) made this pull request unmergeable. Please resolve the merge conflicts.

code0100fun · 2016-03-28T08:05:04Z

packages/ember-glimmer/tests/utils/abstract-test-case.js

-      this.assertSameNode(newSnapshot[i], oldSnapshot[i]);
+      if (!(newSnapshot[i] instanceof TextNode && oldSnapshot[i] instanceof TextNode)) {
+        this.assertSameNode(newSnapshot[i], oldSnapshot[i]);
+      }


It looks like escaped content creates a new node instead of updating the node value. This happens in htmlbars and glimmer. Not sure if this is intended or expected behavior or a bug. For now this is how I have made the assertInvariants pass for escaped text content.

I think this is fine for now as long as we clean it up before merge. I'll try to come up a suggestion. I have a few ideas, but I need to look into the details to know if they would work.

As for your question, this is expected. The content inside a triple curlies are parsed and populated by the browser. If the source string changes, we have to wipe out the existing nodes and have the browser re-create them. So this is currently just a definciency in the test abstraction, not a bug in the implementation.

Moved to content tests.

code0100fun · 2016-04-04T02:06:48Z

packages/ember-glimmer/tests/integration/content-test.js

@@ -244,6 +340,14 @@ moduleFor('Dynamic content tests (content position)', class extends DynamicConte

 });

+moduleFor('@htmlbars Dynamic content tests ("trusted" curlies)', class extends DynamicContentTest {


These tests are currently failing on IE9/10/11. Tagged them for HTMLBars for now.

code0100fun · 2016-04-04T02:19:03Z

I couldn't figure out a clean way to integrate the escaped HTML tests into the test matrix.

All the existing values in the matrix are tested in {{{triple curlies}}} but there are some glimmer failures on IE.

I left explicit tests of HTML content mainly because assertInvariants was causing issues with HTML content and I don't have a clear understanding of how to refactor the test runner to account for these changes.

@chancancode, hopefully this is close to acceptable. Sorry you've had to sink so much time on this 😅

chancancode · 2016-04-04T05:43:31Z

Thank you for your work on this! I'll take a look tomorrow!

[Glimmer2] Move escape tests to `ember-glimmer`

chancancode · 2016-04-05T21:11:03Z

@code0100fun Thank you again for your work, I merged this in #13259. This turned out to be a bit more difficult to abstract than I expected, because it's testing something pretty low-level. HTMLBars tend to have more "marker" elements than Glimmer (empty text nodes, etc), but I think I have found an okay path. It's still not as nice as I wanted it to be, but it's good enough for now! (You can see the result in #13259.)

If you or other people have better ideas for those abstractions, feel free to open subsequent PRs to improve it.

code0100fun reviewed Mar 20, 2016
View reviewed changes

code0100fun force-pushed the glimmer-escape-tests branch from 64e7589 to c8a0e64 Compare March 20, 2016 23:22

code0100fun mentioned this pull request Mar 20, 2016

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

Closed

45 tasks

code0100fun changed the title ~~[Glimmer2] Move escape tests to ember-glimmer~~ WIP [Glimmer2] Move escape tests to ember-glimmer Mar 20, 2016

code0100fun force-pushed the glimmer-escape-tests branch from c8a0e64 to c4fae53 Compare March 20, 2016 23:53

code0100fun reviewed Mar 21, 2016
View reviewed changes

code0100fun force-pushed the glimmer-escape-tests branch 3 times, most recently from fb8e091 to aa0b832 Compare March 22, 2016 12:43

chancancode reviewed Mar 24, 2016
View reviewed changes

code0100fun force-pushed the glimmer-escape-tests branch 2 times, most recently from 0c59b0b to 6016ed1 Compare March 27, 2016 20:56

code0100fun reviewed Mar 28, 2016
View reviewed changes

code0100fun force-pushed the glimmer-escape-tests branch 4 times, most recently from f45a4e9 to 0a736f2 Compare April 3, 2016 05:26

code0100fun changed the title ~~WIP [Glimmer2] Move escape tests to ember-glimmer~~ [Glimmer2] Move escape tests to ember-glimmer Apr 3, 2016

[Glimmer2] Move escape tests to ember-glimmer

aa12b52

Moved to content tests.

code0100fun force-pushed the glimmer-escape-tests branch from 3d4652f to aa12b52 Compare April 4, 2016 02:04

code0100fun reviewed Apr 4, 2016
View reviewed changes

wycats pushed a commit that referenced this pull request Apr 5, 2016

Merge pull request #13143 from code0100fun/glimmer-escape-tests

eb42265

[Glimmer2] Move escape tests to `ember-glimmer`

chancancode closed this Apr 5, 2016

code0100fun deleted the glimmer-escape-tests branch April 6, 2016 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Glimmer2] Move escape tests to `ember-glimmer` #13143

[Glimmer2] Move escape tests to `ember-glimmer` #13143

code0100fun commented Mar 20, 2016

code0100fun Mar 20, 2016

chancancode Mar 24, 2016

code0100fun Mar 21, 2016

chancancode Mar 24, 2016

chancancode Mar 24, 2016

chancancode commented Mar 24, 2016

chancancode Mar 24, 2016

homu commented Mar 27, 2016

code0100fun Mar 28, 2016

chancancode Mar 29, 2016

code0100fun Apr 4, 2016

code0100fun commented Apr 4, 2016

chancancode commented Apr 4, 2016

chancancode commented Apr 5, 2016

		@@ -244,6 +340,14 @@ moduleFor('Dynamic content tests (content position)', class extends DynamicConte

		});

		moduleFor('@htmlbars Dynamic content tests ("trusted" curlies)', class extends DynamicContentTest {

[Glimmer2] Move escape tests to ember-glimmer #13143

[Glimmer2] Move escape tests to ember-glimmer #13143

Conversation

code0100fun 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

chancancode commented Mar 24, 2016

Choose a reason for hiding this comment

homu commented Mar 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code0100fun commented Apr 4, 2016

chancancode commented Apr 4, 2016

chancancode commented Apr 5, 2016

[Glimmer2] Move escape tests to `ember-glimmer` #13143

[Glimmer2] Move escape tests to `ember-glimmer` #13143