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

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

Closed
40 of 45 tasks
chancancode opened this issue Mar 19, 2016 · 44 comments
Closed
40 of 45 tasks

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

chancancode opened this issue Mar 19, 2016 · 44 comments

Comments

@chancancode
Copy link
Member

chancancode commented Mar 19, 2016

♻️ ♻️ ♻️ Please consider the environment before printing this Github issue. ♻️ ♻️ ♻️

The Backstory

@wycats and I (and many other people who have helped along the way) have been working on a rebuild of the rendering engine for the last six months, codenamed "Glimmer 2".

It started as a fork of htmlbars but almost every line of code has been rewritten (sometimes several times) by now. We distilled all the learnings from building the previous-generation rendering pipelines (handlebars, htmlbars, the original glimmer project, etc), resulting in an architecture that's simultaneously more fitting for Ember's use cases but is also more flexible and well-positioned for future extensions and other non-Ember use cases.

You can find the codez at https://github.com/tildeio/glimmer. It's (re)written in TypeScript, and I think it's pretty cool. Anyway, more on that at EmberConf.

The Integration

While there are still a lot of work (mostly optimizations) we would like to do on the engine, we believe we have implemented enough to cover all the basic Ember use cases. So about two months ago, we have started integrating the new engine into Ember proper. You can follow this meta-issue for our progress so-far.

While it is not yet possible to use the new engine in real apps just yet, we expect that work to be completed relatively soon. The expectation is that once we have finished the implementation, it will be a relatively painless, drop-in upgrade for apps, just like the original Handlebars to HTMLBars migration.

(It should be noted that the ability to toggle the feature flag will probably land before all the existing features are implemented, so it will probably not work seamless with your app from the get-go.)

Pls send halp

So, you might be wondering, "How can I help?"

I am glad you asked! 💥 ✨ 🎆 🎉

At this point, the highest value you can do to help is to help port the existing tests (and help reviewing these PRs). You see, Ember has a pretty extensive test suite that tests the "view layer" behavior. The problem is that a lot of these tests are written in ways that are quite coupled to the existing implementation or otherwise uses legacy semantics (such as {{view.foo}}) that are no longer supported.

In order to be certain that we did not cause any regressions, we would like to be able to run our entire test suite against both the current rendering engine ("htmlbars") and Glimmer 2.

We have written a new test harness that allow us to do just that. I will get into the technical details below, but the basic idea is that we have written our test cases against an abstraction layer that encapsulates the differences between the two engines, allowing the same code in the test cases to run against both implementations.

Along the way, we are also "modernizing" the existing tests in the process to not rely on legacy semantics (unless they appear to be explicitly testing those semantics, in which case we will leave them alone). Our new test harness also makes it much easier and much more pleasant to do "matrix style" tests. More on that below, but here is a high-level architecture diagram:

matrix

The net result is that the tests are now much easier to read and reason about, and we have also greatly increased coverage. This is great outcome for everybody, but we still have a lot more tests left, and we cannot feel confident shipping the engine without all of them being ported. However, if a few of you could help us port a test file each, we will be in very good shape by this time next week!

How the harness works

The actual mechanism we used is pretty low tech. You might have heard of it, it's called symbolic links.

Inside the test folder of ember-glimmer package, you will find a file called abstract-test-case.js, which is also symlinked to the same location inside the ember-htmlbars package. This file defines the APIs we use the write the test cases. Because this file is shared (symlinked) between both packages, it does not contain anything specific about the two implementations.

Instead, all the differences are abstracted by importing files (such as import ... from './helpers') provided by each package. Alternatively, each package can also override specific methods on the "abstract" classes in test-case.js (see the ember-glimmer vs ember-htmlbars versions).

(In a lot of cases, you might not even need to modify these files at all, but knowing that's how it works/where the work happen under-the-hood might be still be helpful if you run into issues.)

How the tests work

Since the engine is intended to be a drop-in upgrade for real apps, as long as we the tests are actually testing how these features are supposed to be used in the real world, there are no reason why the tests wouldn't run with both engines.

That's been our focus so far. You can see an example here.

This test is physically located inside the ember-glimmer directory, but it is symlinked to the same location in the ember-htmlbars directory (in fact, the entire directory is symlinked).

As you can see, the test imports this package-specific test-case.js, but otherwise is agnostic about the rendering engine implementation.

The process

In general, and at the high-level, the process looks like this:

  1. Pick a test file to port (usually it's an existing test file somewhere in ember-htmlbars)
  2. Make a new file inside ember-glimmer/tests/integration/... somewhere
  3. Port each test case/module to the new file, while...
    • Using the new moduleFor and ES6 class format
    • Ensuring that each test go through the "I-N-U-R" cycle ("initial render -> no-op rerender -> update(s) via mutation(s) -> reset via replacement" cycle, more on this below)
    • Removing (ignoring) duplicated tests (or tests that are implicitly covered by the update cycle mentioned above)
  4. Symlink the test back into the ember-htmlbars package, unless the parent folder is already a symlink in ember-htmlbars (like the concat test I showed above)
  5. Remove the old file (unless it still contains some tests that cannot be ported)
  6. Open a pull-request
  7. To make it easy to review, add a line comment for each test case you removed, stating the rationale (e.g. it was ported to /it is now covered via /it was a duplication of /...). You can also feel free to add comments/questions on tests you are not sure about.

How to write good tests

Here are a few general tips/rules that you can follow to improve the test cases.

The "I-N-U-R" cycle

We would like each test to go through the "I-N-U-R" cycle:

  1. Initial render

    Render the template you want to test, with the initial values of your choice (this.render(..., { ... })) and assert the result is what you expect. (Example)

  2. No-op re-render

    Call this.runTask(() => this.rerender()); without any changes to the values, then assert that the result remain the same. (Example)

  3. Update(s) via mutation(s)

    Make some updates to the values you use in the templates. (Example)

    You should try to:

    • Break up the updates into multiple chunks (i.e. multiple this.runTask + assertion) if it makes sense. This increase chances of catching "clobbering" bugs where updating some of the values will "blow away" another unrelated part of the template or causes other undesirable effects.
    • Use "interior mutations" if it makes sense. When the value is just a string or other primitive value, this doesn't matter, but when you are dealing with an object or an array, this means updating the values "inside" of the object/array while keeping the object/array the same. (Array Example, Object Example)
    • Try different forms of "interior mutations" if it makes sense. When there are more than one ways to do this (e.g. pushObject vs removing an item, etc), it's usually a good idea to try more than one of them. (Example)
  4. Reset via replacement

    Reset to the original starting condition by replacing all the variables.

    • Reset: this helps to catch bugs where we caches the original value of the text node and forgot to update the cache along the way. In that case when you change it to anything other than the original value, it would work fine; but when you change it back to the original value it will short-circuit the DOM code and do nothing.
    • Replacement: again, if the values are simple primitive values like strings, then this doesn't make a difference. But if the values are objects/arrays, etc, this means replacing that object/array with another new object (as opposed to mutating its internal values). (Array Example, Object Example)

Avoid duplicating tests

It is easy to copy a test case a few times to test slightly different variations of the same thing (e.g. {{#if foo}} starting with true vs false, or the difference between a "POJO" vs an Ember.Object), and we have done that a lot in the existing tests.

Sometimes that's the best you can do, but there are a lot of problems with this. First it produces a large amount of tests physically in the file, making it hard to find things. Also, when someone need to add a new test, they will usually randomly pick one of the few variants, carrying over details/mistakes that doesn't make a lot of sense for the new scenario. When we fix bugs in one of the copies, we will probably forget the rest.

Usually, there are ways to avoid the duplication. For example, in the case of testing the different starting condition ({{#if foo}} against true and false), you can just test both starting conditions in the same test:

["@test if"]() {
  this.render(`{{#if cond1}}T{{else}}F{{/if}}{{#if cond2}}T{{else}}F{{/if}}`, { cond1: true, cond2: false });`

  ... // follow the usual I-N-U-R cycle
}

In other cases, we have been able to define shared behaviors by extracting some shared super classes (putting the actual test cases in the super class), and configure the parts that are different in the subclass. This allows you to write the test cases once, and automatically have them run many different scenarios (the "Matrix Style" tests).

The best example is probably the conditionals test (if, unless, etc). The actual test file simply defines the template invocation style and subclass TogglingSyntaxConditionalsTest (located in shared-conditional-tests.js) and automatically get many shared tests.

The "inline if/unless" tests takes this even further, running the same set of test cases against 11 (!) different invocation scenarios.

The actual structure of the sharing was somewhat difficult to arrive at, and took some time to mature/get right, but the payoff there was massive (the basic scenarios are now shared between {{#with}} and {{#each}} as well).

Legacy semantics

A lot of the existing tests uses legacy semantics like views, {{view.foo}}, {{#view}}, context, controllers, etc. Most of the times, this is purely incidental, and just a result of the test being written in a time where those primitives were the main way to do things. In those cases, you can usually port them to the new harness (which uses components instead) without problems.

When in doubt, you can also a test unported in the first iteration of your PR and ask your questions in a line comment.

To attrs or not to attrs

We decided to default to not using {{attrs.foo}} in tests that uses curly components (which is almost all of them) and rather rely just relying on the attrs being reflected onto the property with the same name (i.e. just {{foo}}). Unless the test is specifically about testing attrs.* (those should probably all be in the same file), you should generally prefer {{foo}} rather than {{attrs.foo}} for consistency. You can always name things like innerFoo vs outerFoo if you feel the need to disambiguate.

See also https://locks.svbtle.com/to-attrs-or-not-to-attrs by @locks.

Caveats

Sometimes you the tests that you ported might not work on either Glimmer 2 or HTMLBars. (If it doesn't work on either engine, you probably did something wrong.)

In the case where it doesn't work on Glimmer 2, it's probably because we haven't implemented that feature completely yet. (For example, we have implemented basic components support but haven't implemented attributeBindings at this point.)

In this case, it is still good to port the test to the new style, so that we can simply enable it when the feature is implemented. To disable a test temporarily for Glimmer 2, you can simply replace the @test prefix in the method name to @htmlbars (which means "run this in HTMLBars-only"). You can also disable an entire module by prefixing its moduleFor name with @htmlbars.

In some rare cases, the test will work correctly on Glimmer 2 but does not pass on HTMLBars. You should double check that the semantics you are testing is correct, but it is also quite possible that there is simply a bug in the current implementation of HTMLBars. (This usually happens when we test some HTMLBars features with the new "matrix style", where the values do not update correctly in some edge cases.)

In that case, you can similarly flag an individual test case or an entire module as @glimmer. Since this is expected to be quite rare (and might require a bug fix), it would be helpful if you can include a brief description of the problems you encounter in a line comment.

Examples

Here are some of the great examples where our community members have helped port existing tests:

As you can see, the earlier iterations was more difficult (we were still figuring out the shared test cases story), but the latter attempts were relatively straight forward. Thank you @GavinJoyce and @chadhietala for paving the way!

So... where do I start?

Here is a list of good starting points. If you are serious about working on one of these, you probably want to leave a comment below so that other people will know not to work on that. (If you ran out of time or encountered great difficult, please come back to "release the lock" and/or push your WIP work, so other people can pick it up!)

There are probably other tests that needs to be ported too. If you noticed anything I missed, please mention them in the comments.

Reviews

Once you are ready to submit your PR (please feel free to submit WIP PRs!), please reference this issue in your PR description, so that we can review them.

Timeframe

We want to get as many test ported as soon as possible. Ideally, we would like to have most (if not all) of the tests ported within the next week or two.

Thank you in advance for your help! ❤️ 💛 💚 💙 💜

@GavinJoyce
Copy link
Member

@krisselden
Copy link
Contributor

I'll look at the "willDestroyElement" tests.

The key thing is it is supposed to be the inverse of didInsertElement so main thing is it runs before DOM teardown, so unlikely that this is covered by willDestroy which is async after DOM teardown. It also is supposed to only run if the didInsertElement hook ran.

@GavinJoyce There is a current bug in htmlbars with this lifecycle hook firing too late in the component helper. #13028

It also is buggy with the current each/else #12716

It also regressed parentView being available at 1.13 but that is private API and has been like that now for a while, not sure though if it is a reason for people being stuck.

Are there other tests covering lifecycle in glimmer? Probably should add them to any test that add/removes components. /cc @wycats @chancancode

@code0100fun
Copy link
Contributor

@chadhietala
Copy link
Contributor

Confirmed that the unported #with test can be deleted.

@green-arrow
Copy link
Contributor

@chadhietala
Copy link
Contributor

I can take unbound 🔒

@mmun
Copy link
Member

mmun commented Mar 19, 2016

I'll port the each-in tests.

@green-arrow
Copy link
Contributor

@chancancode - I think we can check off / remove the debug tests item as well.

@zackthehuman
Copy link
Contributor

  • custom-helper-tests.

@lorcan
Copy link
Contributor

lorcan commented Mar 20, 2016

#13139 removes the unused glimmer-component tests folder

@chancancode
Copy link
Member Author

I'm taking "Basic content rendering tests" (and fixing the implementation in Glimmer)

@HeroicEric
Copy link
Sponsor Member

I'm taking "select test ✂️"

@code0100fun
Copy link
Contributor

code0100fun commented Mar 20, 2016

Updating to match style introduced in 5c12157

@HeroicEric
Copy link
Sponsor Member

@paddyobrien
Copy link

I'm taking a look at input element tests if they're still not locked.

@chadhietala
Copy link
Contributor

I'll take the view helper tests and all the things that it touched.

@chancancode
Copy link
Member Author

Just wanted to jump in and thank everyone for helping us out! 😄 My apologies for the delay – we are slowly digging ourselves out of the backlog; we are closer than it appears on Github's progress bar, because a lot of the 🔒ed items already have PRs waiting for review 🎉

@Joelkang
Copy link
Contributor

Joelkang commented Apr 14, 2016

I'll take the {{#each}} test: #13349

@asakusuma
Copy link
Contributor

I'll take the "local lookup" test

@Joelkang
Copy link
Contributor

Joelkang commented Apr 18, 2016

it looks like the system/lookup-helper_test.js file is testing the actual findHelper method, which appears to me to be covered by integration/helpers/custom-helper-tests.js. Doesn't seem to me that we're unit testing the actual ember-glimmer lib, so maybe ✂️ ? @chadhietala @asakusuma since both of you touched helper-lookup-related tests can you confirm?

@asakusuma
Copy link
Contributor

@Joelkang I can't recall anything related to your question, what exact files have I touched that are related? If I can look at the git commit where I touched it, might jog my memory.

@Joelkang
Copy link
Contributor

@asakusuma oh i just meant that since you're working on the local lookup test, to see if there's any commonality there at all

@asakusuma
Copy link
Contributor

integration/helpers/custom-helper-tests.js doesn't appear to be testing local lookup. Also, local lookup is not working in glimmer right now, which I'm working on fixing.

@locks locks added the Glimmer2 label Apr 19, 2016
@mixonic
Copy link
Sponsor Member

mixonic commented Apr 22, 2016

render env tests are snipped. Looking at "bootstrap" tests now, many of which do need to be ported with the functionality (using <script type="text/x-handlebars" data-template-name="foo">).

@Joelkang
Copy link
Contributor

Joelkang commented May 5, 2016

Did a simple migration of mutable bindings here: #13456

@Serabe
Copy link
Member

Serabe commented May 8, 2016

Closure components tests were already merged a couple of weeks ago.

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2016

Thank you all for the hard work here! Closing this in favor of an updated listing/issue: #13644.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests