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

Move to Webpack+Karma for the testing environment #349

Closed
Reinmar opened this issue Oct 19, 2016 · 10 comments
Closed

Move to Webpack+Karma for the testing environment #349

Reinmar opened this issue Oct 19, 2016 · 10 comments
Assignees
Labels
type:task This issue reports a chore (non-production change) and other types of "todos".
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Oct 19, 2016

We need to switch from Bender.js to a more popular setup for the main two reasons:

  • The ease of configuring CI.
  • To make the whole setup more familiar to other developers.

We have already experience with Karma and Webpack and this seems to be one of the most popular setups so we decided to use them for CKEditor 5.

@Reinmar Reinmar added this to the iteration 4 milestone Oct 19, 2016
@Reinmar Reinmar added status:confirmed type:task This issue reports a chore (non-production change) and other types of "todos". labels Oct 19, 2016
@Reinmar
Copy link
Member Author

Reinmar commented Oct 19, 2016

Notes from my call with @pomek:

  1. Code coverage – use the same method as PJ.
  2. Try to load ES6 modules to Webpack. Remove the es6->cjs transpiler. Use:
  3. Instead of preventing compiling other packages tests, compile all tests (of all packages) and configure Karma to run just the tests that the developer requested.
  4. Performance:
    • startup performance (doesn't have to be great),
    • runtime performance (should be great but requires flawless watch mode).
  5. The watch mode:
    • Check how you can run tests from the browser, not from the console.
    • Test Webpack watcher with compiler --watch (doesn't have to be one gulp task).
  6. Fix failing tests (or rather – prepare them for Webpack and Karma):
    • Convert all tests which use config.features with strings to imports.
    • load.js & module.js
      1. First to check – do we have System when babel-loader is used. Because if we have it, then we could implement load__esnext.js (actually, it's ready) and modules__esnext.js (would have to be done).
      2. If there's no System or it doesn't work with Webpack (wouldn't be strange, cause it must work on runtime... although Webpack has such functionality), then we could totally remove the load.js stuff and the whole module.js. They only make sense in the source mode, but then you can easily use imports and you don't need loading features by strings. Side note: We have to define anyway how can you instantiate two editors with a different set of features, when using one bundle. Related tickets: https://github.com/ckeditor/ckeditor5-dev-bundler-rollup/issues/1 and https://github.com/ckeditor/ckeditor5-core/issues/28 and https://github.com/ckeditor/ckeditor5-core/issues/27#issuecomment-251687924.

@Reinmar Reinmar assigned Reinmar and pomek and unassigned Reinmar Oct 19, 2016
@Reinmar
Copy link
Member Author

Reinmar commented Oct 19, 2016

As for Webpack+ES6 modules, there's an updated article in https://leanpub.com/setting-up-es6/read#sec_webpack-babel.

@Reinmar
Copy link
Member Author

Reinmar commented Oct 19, 2016

OK, there's is a chance that what they recommend (babel-loader) transpiles the whole code to ES5 (which we don't want to do). We want to transpile only the modules, so perhaps using transform-es2015-modules-commonjs makes more sense. However, the problem with our approach now is that we use that tranpiler on the Webpack side while CKEditor's compiler can produce CJS output itself.

@Reinmar
Copy link
Member Author

Reinmar commented Oct 19, 2016

Update from @pjasiun: babel-loader must always be used and it only allows you to use Babel's plugins in Webpack. Now – @pjasiun correctly pointed out that it makes more sense to transpile modules in Webpack because then source maps produced by Webpack shows the original code, not code which was already transpiled by CKEditor's compiler.

So, most likely you had this right, @pomek and we've just read the code incorrectly. Let's keep it as you have it now.

As for code coverage, @pjasiun didn't have any issues like we with uncovered branches, despite no branches in the code. So please check the method that he's using.

@Reinmar
Copy link
Member Author

Reinmar commented Oct 19, 2016

I reported a ticket for removing the load.js functionality: https://github.com/ckeditor/ckeditor5-core/issues/32

@pomek
Copy link
Member

pomek commented Oct 21, 2016

Let me say few words as an update for this ticket.

Generally, Karma and Webpack works! We can test the whole editor:

image

or particular package (for testing I choose the ckeditor5-basic-styles):

image

At this moment, all code is duplicated (ckeditor5 and ckeditor5-basic-styles contain the same tasks). It was, let say - PoC. Everything works as I expected. We can focus on polishing and refactor stuff and move the whole code to correct place, repositories.

Unfortunately, I couldn't configure generating the code coverage with Karma and Webpack.
But… After few hours battle between me and Karma, I won!

We can test all the things using Karma and generate CC for:

basic-styles:

image

the whole editor (unfortunately, I cannot show you):

image

image

@Reinmar
Copy link
Member Author

Reinmar commented Oct 21, 2016

WAT... Webpack/Karma/Chrome uses all the available memory? It's not even about missing RAM, but most likely Node's limitation (1GB AFAIR).

Buuuuut. I don't think this is a problem. We should strive to test every package separately and this works fine and is quick. If you make a PR to the engine and there are 10 dependent packages, the CI will need to run tests for all those packages and count their CC separately. So, you'll never need to run CC for all the packages combined.

Besides, you should never do this because then one package's tests affect CC of other packages.

@Reinmar
Copy link
Member Author

Reinmar commented Oct 21, 2016

OTOH, I can hardly believe that Webpack+Karma are not able to fit in 1GB ;/ Sounds crazy. How many modules do we have now? Cloc says:

-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Javascript                    1160          24698          32713          76526

Is it really too much for Webpack?

@pomek
Copy link
Member

pomek commented Oct 21, 2016

We should strive to test every package separately and this works fine and is quick.

I agree. We can run the whole tests from the ckeditor5 repository but CC we should check each package separately.

@pomek
Copy link
Member

pomek commented Oct 25, 2016

I have nothing to say. I just paste the gif:

ckeditor5-karma

Only one test does not pass (in ui-default) but we work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

3 participants