search

tomdale / ember-cli-addon-tests

31 stars 18 forks source link

Modernize the addon #220

Open jelhan opened 4 years ago

jelhan commented 4 years ago

I like this addon for the declarative API it provides. But to be honest it's neither stable nor fast. Not sure if I'm using it wrong in ember-cli-content-security-policy but it's very instable. Nearly every change causes it to blow up. And sometime it's very hard to reproduce same behavior locally and on CI. I guess I'm not the only one facing these issues?

But I'm also not a big fan of the alternatives. Some addons use a monorepository with a huge amount of different packages for testing. ember-auto-import is an example of that approach. In my opinion it's very hard to reason about what each package is for. It's a huge duplication of code. I'm wondering how this could be maintained.

Long story short: I see a lot of problems with the current state of the addon but at the same time still a need for the features it provides.

I think it's time for a modernization. Before starting that work I like to make sure that I'm running in the right direction. I'm seeing different steps:

  1. Remove features that aren't needed anymore in modern Ember.
  2. Use established APIs to symlink the addon under test and to address performance issues instead of custom solutions.

Let me go through these topics separately and discuss them in detail.

Remove features that aren't needed anymore in modern Ember

A good amount of the code (and the complexity) is related to Bower support. It even supports depending on ember-source Bower package. I think all that code can be removed.

I'm not sure if there is other code related to Ember features with have been deprecated and removed long time ago. Haven't gone through the full code base. But I think as a rule of thumb we should remove all features, which are not required to test the latest two LTS versions. This should make a refactoring of the remaining parts way easier.

Use established APIs to symlink the addon under test and to address performance issues instead of custom solutions

To allow testing an addon in many different apps ember-cli-addon-tests tries to speed up the creation of new apps as much as possible:

This package automates the process of creating a new Ember CLI app and caching its npm and Bower dependencies, so each test run can get a fresh app in very little time.

It does so by creating an application once with ember new. Applications, which are used for testing, are created from this pristine app by cloning it's files to another temporary location.

Using yarn workspaces should give us similar performance but without the added complexity. I have performed some local tests and the performance of ember new in a yarn workspace is promising.

On my local machine first run took 1 minute 20 seconds. All subsequents runs in the same workspace took less than 5 seconds. I expect it to be even faster as the current approach cause no need to clone the heavy node_modules folder. But I haven't done a comparable benchmark between both of them.

Using yarn workspaces has two advantages beside possible performance improvements in my perspective:

  1. It uses an established technology. So it's less likely that we face issues with different operation systems, file systems or similar stuff.
  2. It's build on top of an established solution in Ember ecosystem. It should be easier to understand and reproduce what is going on in the tests compared to the current solution.

Main trade-off of relying on yarn workspaces would be dropping support for npm. But currently it's the other way around: The custom solutions that are currently used do not support yarn.

Another custom solution is the way how ember-cli-addon-tests links the addon under test to the application used for testing. It tries to simulate npm link by creating a symlink for the addon inside of the application's node_modules folder. I expect some of the stability issues I'm facing to be related to this one.

Official features of the package manager should be used for this one as well in my opinion. It should work by adding the addon as a regular dev-dependency to the application using file: or link: protocol.

Please let me know if you agree with the proposed direction for this addon. If so I will try to find some time to provide small merge request to incrementally land the changes.

xg-wang commented 3 years ago

Embroider and ember-auto-import leverages https://github.com/ef4/scenario-tester, could be an alternative or the new primitive to be built upon.