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 again!] Test Porting Guide #561

Closed
7 of 9 tasks
chadhietala opened this issue Jun 28, 2017 · 13 comments
Closed
7 of 9 tasks

[Pls send halp again!] Test Porting Guide #561

chadhietala opened this issue Jun 28, 2017 · 13 comments

Comments

@chadhietala
Copy link
Member

chadhietala commented Jun 28, 2017

Requires #560

Backstory

The repository originally started off as being a fork of HTMLBars and thus we inherited the test suite. While many tests have been added the test suite has grown somewhat organically over the past year and half. We have seen several test harnesses be introduced making it hard to understand exactly how to introduce new tests 😞 . Luckily we have landed on a test harness that we really like and need halp migrating the rest of the test suite to use it 🤞 😄 🤞 .

Turning On The Bat Signal 🦇

People from the internet, we need your halp! This effort is similar to the effort when we originally introduced the Glimmer VM into Ember. At this point, the highest value you can do to help is to port the existing tests (and help reviewing these PRs). If you are familiar this the new test harness in Ember you should feel right at home 🏠 . If not, no reason to be worried, we have outlined the migration steps below.

The Process

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

  1. Pick a test file to port (will be found in packages/@glimmer/runtime/test)
  2. Determine if you can break apart the tests into a more meaningful file
  3. Import RenderTests and module from @glimmer/test-helpers
  4. Extend the RenderTest creating your own subclass
    • Port each tests in the file and module
    • Make sure you follow the I-N-R-U cycle where appropriate
    • Remove duplicated tests 😬
  5. Pass the new subclass into module as such module('NAME OF MODULE', MyTestClass)
  6. Remove the old test file
  7. Create a PR
    • If you have collapsed or deleted test cases please leave comments in the Github issue explaining what was collapsed. This typically is done when the I-N-R-U cycle was spread across multiple test cases.

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 template and initial values of your choice (this.render('<h1>Hello {{name}}</h1>', { name: 'Chad' })) and assert the result is what you expect. (Example)

  2. No-op re-render

    Call this.assertStableRerender(); to assert that a no-op re-render is stable. (Example)

  3. Update(s) via mutation(s)

    Make some updates to the values you use in the templates and assert the result. You should also assert that the DOM nodes remain stable through the update. (Example)

    You should try to:

    • Break up the updates into multiple chunks (i.e. multiple this.rerender + 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.
  4. Reset via replacement

    Reset to the original starting condition by replacing all the variables. 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.

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), 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.

Examples

Here is an example that shows how we have tested initial render and rehydration.

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.

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!)

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! ❤️ 💛 💚 💙 💜

Thanks @chancancode for providing the Ember issue so I could copypasta from it 🍝

@Serabe
Copy link
Member

Serabe commented Jul 3, 2017

Starting with Attributes Tests

@Serabe
Copy link
Member

Serabe commented Jul 9, 2017

#564 is ready for review. I'm starting with Ember components.

@chadhietala
Copy link
Member Author

@Serabe Porting the Ember Components might be tricky as we need to create some abstraction for testComponent that runs unit under various conditions. Let me try to get something working.

@snewcomer
Copy link
Contributor

me and @crodriguez1a are working on Style Attributes test.

@piotrpalek
Copy link
Contributor

piotrpalek commented Jul 12, 2017

@chadhietala this might be slightly out of topic, but I tried to npm i in the main directory and had this error:

npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated [email protected]: This package is discontinued. Use lodash@^4.0.0.
npm WARN deprecated [email protected]: This package is discontinued. Use lodash@^4.0.0.
npm WARN deprecated [email protected]: Use uuid module instead
npm ERR! Linux 4.11.8-300.fc26.x86_64
npm ERR! argv "/usr/bin/node" "/home/peter/.npm-packages/bin/npm" "i"
npm ERR! node v6.11.0
npm ERR! npm  v3.10.8
npm ERR! code ETARGET

npm ERR! notarget No compatible version found: broccoli-typescript-compiler@next
npm ERR! notarget Valid install targets:
npm ERR! notarget 2.0.0-beta.1, 1.0.1, 1.0.0, 0.6.2, 0.6.1, 0.6.0, 0.5.0, 0.4.1, 0.4.0, 0.3.0, 0.2.1, 0.2.0, 0.1.2, 0.1.1, 0.1.0, 0.0.6, 0.0.5, 0.0.4, 0.0.3, 0.0.2, 0.0.1
npm ERR! notarget
npm ERR! notarget This is most likely not a problem with npm itself.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.
npm ERR! notarget
npm ERR! notarget It was specified as a dependency of '@glimmer/build'
npm ERR! notarget

I was a bit confused on what I've done wrong (tried rm -rf node_modules && npm clear cache) but tried yarn after that and it worked. Might be a good heads up to anyone who encounters the same issue.

@tomdale
Copy link
Contributor

tomdale commented Jul 12, 2017

@krisselden What version of broccoli-typescript-compiler should we be using?

@krisselden
Copy link
Contributor

the tag is @beta I can add a next tag, though I'll probably just release since everyone who has tried it has said it worked well for them.

@tomdale
Copy link
Contributor

tomdale commented Jul 13, 2017

@krisselden 👍 on a release

@rwjblue
Copy link
Member

rwjblue commented Jul 14, 2017

Agreed. Seems good to release to me also.

@piotrpalek
Copy link
Contributor

Started porting in-element-tests related PR: #583

@sduquej
Copy link
Contributor

sduquej commented Jul 19, 2017

Myself and @GavinJoyce are looking into Debugger tests

@snewcomer
Copy link
Contributor

@chadhietala By and large is this issue still relevant? If so, I can help with two of the items above.

@Turbo87
Copy link
Member

Turbo87 commented Feb 28, 2020

as there hasn't been any activity here for over 2.5 years I'll go ahead and close this issue as apparently most of the todo items have been resolved by now.

if that is an incorrect assumption feel free to reopen the issue :)

@Turbo87 Turbo87 closed this as completed Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants