[Glimmer2] Component invocation tests

[Serabe]

This PR explores the use of Glimmer RenderingTest and moduleFor, by @chancancode and @wycats, for Ember testing. This was originally a duplicate effort with a similar DSL for test, though it used plain QUnit.

First file to be ported has been component_invocation_test.js, though we have only changed superficial methods and I'll go through the file again to use some of the assertions included in RenderingTest.

A few comments on the experience:

1. I would make render() and rerender() to be wrapped inside a run by default.
2. I needed to change the behaviour of templates to be accepted compiled. A few tests depended on passing a given environment to the compiler.

For the render() and rerender() case, I think it is easier to read:

// Test setup

this.render('{{my-component}}');

// Some assertions

than:

// Test setup

Ember.run(() => {
  this.render('{{my-component}}');
});

// Some assertions

If for any reason the test needed to render the component without being inside a run, this.component.render() would be enough.

And just curious, why are all test names surrounded by [], babel seems to be ok with just '@test something'(){}

Old PR
This PR adds a module to improve internal testing. The final purpose is to
avoid the use of Views for testing.

This PR is in WIP until:

• A name for moduleForApp has been chosen.
• The API has been approved.

The API has been created to avoid the need of using the internal variables (no more need to use view as the variable to make the teardown work).

API

this.getOwner()

Returns the owner created for this test run.

this.register(fullName, factory, options)

Registers in the owner the factory with the name fullName and the given options. Returns the registered factory.

this.render(template, hash={})

Creates a new anonymous component with the owner for the tests and the given template. It instances said component with the passed hash and appends it to the view.

Currently, template needs to be compiled previously.

The instance is kept to be destroyed in the teardown process.

It returns the component instance.

Calling it more than once makes the test fails

~~ this.rerender()~~

Same as run(component, 'rerender'), where component is the one this.render instantiated.

this.$()

Calls $() in the component created while using this.render().

[Serabe]

I added a rerender method, like in #12891. All attributes are made private and this.render cannot be called more than once in a test.

[homu]

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

[Serabe]

Updated title and description for new approach.

[homu]

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

[chancancode]

@Serabe I'm so sorry I missed this! I'll try to review it during the weekend. (Meanwhile, can you review #13127 for me?)

[chancancode]

I would make render() and rerender() to be wrapped inside a run by default.

I'm not sure if we already added runTask when you started working on this. I think we want to be explicit about that. @wycats?

[chancancode]

I needed to change the behaviour of templates to be accepted compiled. A few tests depended on passing a given environment to the compiler.

That is not implemented in the Glimmer 2 compiler yet, although we can certainly (and most likely have to) bring it back. Is there any chance that we can collect up all those tests in its own file (or module at least) and make them @htmlbars for now?

[chancancode]

And just curious, why are all test names surrounded by [], babel seems to be ok with just '@test something'(){}

Good question. I think both works. @wycats?

[chancancode]

Hey @Serabe! I pulled your changes locally and played with it. I rebased it against current master and pushed the result here with a few changes: tildeio@51fdc08

I think we ran into much of the same problems when we were porting other tests, and we have arrived at some similar solutions. Specifically:

1. We have added this.runTask which is an abstraction over the run loop (it semantically means "this block of code happened inside a event handler / ajax callback, etc" – which is why it's good to be explicit in those cases). Can you update that instead of using Ember.run directly?
2. this.textValue() is intended to mean "what's the text being rendered", similar to this.firstChild or this.element. It really should be a getter, but I don't want to change all the things at this moment. Because of that, I don't think we should patch it to take a selector, so I added a assertTextForSelector that does the thing you want (and updated the tests to use it).
3. I think the moduleForApp stuff is unused? (We have encountered similar requirements and made an ApplicationTestCase, is that roughly what you need also?)
4. [nit] Prefer set(this.context, 'someProp', 'tomdale') over this.component.set('someProp', 'tomdale')
5. [nit] Prefer this.registerFoo(...) over this.owner.register(...)
6. [nit] Prefer this.runTask(() => ...one line...); over run(() => {\n...\n});

Does that make sense?

---

Also, it would be great if you can port the tests to follow the style guide in #13127, in particular the I-N-U-R cycle and probably the attrs. guideline (unless the test is specifically testing the attrs feature).

Do you still want to keep working on this? (Sorry for the huge gap between you submitting this and the review.) I think you have done a lot of great work here, and even if you don't have time to finish this it would be a very helpful starting point for someone else (or us).

[Serabe]

Thank you for your comments. I will address them tomorrow morning (it is pretty late in Spain and I'm on my phone now) but I am still working on them. I have started a branch from master directly and I'm moving tests to curly components test as suggested. Furthermore, I'll take on closure component helper if I have time since I have another WIP somewhere for the glimmer version as well.

[Serabe]

Sorry for the delay, @chancancode. I have a few questions:

1. In the component-invocation file thee are test about GlimmerComponents, is there any preferred file to put those in?
2. In the guide it is explicitly said that attrs.foo should not be used in tests. Should I mark somehow the tests about attrs behaviour?

Thank you!

[Serabe]

Most of the tests are already in this file in the glimmer engine.

[Serabe]

I've push a first batch on tests so reviewing is easier. I have some questions from when I have been porting these:

1. I kept the INUR to IN in some tests (those dealing with static context).
2. There are some tests checking attrs behaviour. Should these be kept or marked as deprecated?
3. I have failing tests for Glimmer for anything related to positional params, how can I help fixing these? (I guess they are not disappearing, though that would make me so happy for all the pain in the past 😝)
4. There are problem with block-related variables (hasBlock and hasBlockParam). I am willing to help on these too if I can help somehow.

[chancancode]

I kept the INUR to IN in some tests (those dealing with static context).

Sounds fine!

There are some tests checking attrs behaviour. Should these be kept or marked as deprecated?

1. If they are unrelated to the attrs feature, you can usually rewrite them into the "bare" form since they are also installed on the component instance. ({{attrs.foo}} -> {{foo}})
2. If they are expressly testing the attrs feature (e.g. `"test {{attrs.foo}} should work", "test {{attrs.nonAttrsPropertyOnTheComponent}} does not work", etc), then leave them as-is
3. If you can't tell, you can leave them as-is too

I have failing tests for Glimmer for anything related to positional params, how can I help fixing these? (I guess they are not disappearing, though that would make me so happy for all the pain in the past 😝)

You can use @htmlbars ... instead of @test ... to skip them on glimmer for now.

There are problem with block-related variables (hasBlock and hasBlockParam). I am willing to help on these too if I can help somehow.

I think @mmun has an implementation that he will publish soon

[homu]

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

[Serabe]

@chancancode, I need some help with a test. I ported this test in old component invocation to this piece of code. It does not work for glimmer, since positional parameters are not yet supported. The sad part is that, though the original tests passes, partial updates are failing in htmlbars as well, but only if the component is invoked via the component helper. It might be a detail I'm not seeing, but any kind of help would be very much appreciated.

[chancancode]

@Serabe if I understand correctly, I think @GavinJoyce bumped into this exact problem when porting the component helper tests: https://github.com/emberjs/ember.js/pull/13140/files#r56916691

He ended up commenting out the update part of the test and opened #13158 to track it.

Does that mean this test you are porting is exactly the same as the one he ported? If so, we can just delete this one probably. Otherwise, what he did there is fine here too. Another alternative is we set a this.package inside AbstractTestCase's constructor, so you can do if (this.package !== 'htmlbars') { ... }.

[Serabe]

No, the intent of the test is to check if the dynamic version of positional params work, but it is affected but the same bug. Thank you!

[Serabe]

This is (almost) ready for merge. The only thing left to discuss is two tests I left on component_invocation that need lower level access to be done and I'll need to know what to do with them. In any case, I removed the need of Ember.View.

When this is merged, I'll open a bug since concatenatedProperties are not being concatenated after the this.runTask(() => this.rerender()).

[chancancode]

@Serabe I assume this is deleted without replacement because it's now covered in https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/tests/integration/components/dynamic-components-test.js ?

[Serabe]

Only partly, thought about it but then decided it would be best to do that later once this is accepted and I had the chance to talk to you what would be the best way to approach this (most tests in this file should be shareable between curly-components and dynamic-components tests). I thought this file was already too big to review so adding more complexity to it would not be worth it in a first review. My apologies!

[chancancode]

ZOMG! I just finished reviewing this 🎉

@Serabe thank you for your work, sorry it has taken so long (it's one of the biggest test file!). I think this is basically good to go. The new tests are excellent! Just a few more questions regarding the @skipped tests – I'm nervous about loosing coverage (even temporarily), so I'd prefer that we address them one way or the other before merging.

[Serabe]

@chancancode thanks to you! I've tried to comment the minimum amount of code in those skipped tests, though some still remain skipped because the don't even pass the N phase.

Most of the problems seem to be about parameters in rerenders. For example, concatenatedProperties are no longer concatenated after a rerender (though in the case of classNames they are still applied and I'm not sure if this might be a bug).

I'm concerned about non-block with properties overridden in init, because the N step modifies the text though it seem natural to me that it should be modified (the property is being overriden in init, not in a lifecycle hook of the component). Should I modify the test? If so, INUR would not be INUR in this case.

Thank you so much for your time!

[homu]

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

[Serabe]

Fixed!

[rwjblue]

Thanks @Serabe!

[chancancode]

woooo thank you @Serabe and @rwjblue!