test(e2e): add tests for ES2015 module support

[appsforartists]

(Note: this is a continuation of #2685, because @wesleycho had to abandon it. @dignifiedquire, you already approved that PR. This is the same one, but rebased off the current HEAD.)

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[mdrobny]

@appsforartists as far as I understand @wesleycho said that he can’t contribute to open source currently so I am not sure if even merging his commit (after your rebase) would work. I think that @wesleycho would be ok with just copying his code and adding as yours for the sake of this project, what do you think guys?

[appsforartists]

I'd want to offer him proper credit, and I don't think it's legally kosher to claim his work as my own either.

My thinking is that he offered this code before he had any restrictions, so including his commit in this PR should be fine (and I presume he would have closed the earlier PR if it wasn't); however, I don't want to be presumptuous. @wesleycho, are there any problems with landing code that you wrote before you worked at your current employer?

[mattyclarkson]

Is there any backup plan if we don't hear from @wesleycho? Really keen to have this!

[appsforartists]

I chatted with @wesleycho off-thread. He can't contribute additional code to this PR, but confirmed that his previous PR was licensed to Karma under the CLA. This can be merged whenever a reviewer gives it the 👍.

@dignifiedquire, can you please merge?

[dignifiedquire]

Hey sorry for the delay. I hadn't much time to look at karma in the past months.

Instead of adding a new property just for esmodules, how about we reuse the idea from #2846 to add a type property which would be script for regular javascript files and module for esmodules.
What do you think?

[appsforartists]

SGTM

I'm away this week, but will try to get to this next week.

[dignifiedquire]

@appsforartists I just merged #2846, can you rebase on master please?

[appsforartists]

Rebased.

e2e tests are taking an unreasonably long time locally, so I haven't been able to verify. I took @wesleycho's original tests + my patches from the original PR, and changed his cucumber file to use type: 'module' instead of esModule: true.

[appsforartists]

Looks like Travis is a lie. If you look at the logs, you'll see

Failures:

1) Scenario: �[1mSimple middleware�[22m - �[90mtest/e2e/module-types.feature:6�[39m
   Step: �[1mThen it passes with:�[22m - �[90mtest/e2e/module-types.feature:20�[39m
   Step Definition: �[90mtest/e2e/step_definitions/core_steps.js:152�[39m
   Message:
     �[31mError: Expected output to match the following:
       ..
     Firefox
     Got:
       Firefox 57.0.0 (Linux 0.0.0): Executed 0 of 0 ERROR (0.001 secs / 0 secs)
     �[39m�[90m
         at World.<anonymous> (/home/travis/build/karma-runner/karma/test/e2e/step_definitions/core_steps.js:173:23)
     �[39m

45 scenarios (�[31m1 failed�[39m, �[32m44 passed�[39m)
155 steps (�[31m1 failed�[39m, �[32m154 passed�[39m)
1m17.262s
�[31m>> �[39mfailed tests, please see the output

so it shouldn't have the 💚 .

[dignifiedquire]

works locally for me, so going to merge

[dignifiedquire]

Spoke too soon, the test seems to be failing, is it working for you locally?

[appsforartists]

@dignifiedquire

Took me a bit to figure out how to get the right test to run against the built karma, but I've got that sorted now.

All the right files are being loaded and code is being executed, but the closures being passed into it calls aren't being called.

With these logs:

import { plus } from '../plus.js'

console.log('a')

describe('plus', function () {
  console.log('b')

  it('should pass', function () {
    console.log('c')
    expect(true).toBe(true)
  })

  it('should work', function () {
    expect(plus(1, 2)).toBe(3)
  })
})

You'd see a and b, but not c. describe and it are defined. Any idea what's going on?

[appsforartists]

I tried using the basic tests, and it appears type: 'module' breaks jasmine's ability to identify tests.

This works:

      files = [
        { pattern: 'basic/plus.js' },
        { pattern: 'basic/test.js' },
      ];

but this fails:

      files = [
        { pattern: 'basic/plus.js', type: 'module' },
        { pattern: 'basic/test.js', type: 'module' },
      ];

[appsforartists]

I just tried mocha as well and had the same problem. Current hypothesis: the mocha and jasmine plugins somehow wrap the JS test files, and type: 'module' is being excluded.

[appsforartists]

@dignifiedquire I'm not sure what the purpose of expect(true).to.be.true is in each of these tests, but it seems that other e2e karma suites have them too.

I think you should merge this, even though the tests don't pass. Presumably if they don't pass in cucumber, #2846 doesn't work for Karma users either. We should have a test to demonstrate this breakage and determine when it's fixed.

[Coridyn]

@appsforartists I've had a look and worked out why jasmine isn't running tests from modules.

ES modules run as deferred but karma kicks off the tests in a synchronous script.

This means that karma starts running tests before the modules have been loaded and therefor those tests don't run.

A possible fix for this would be to change the window.__karma__.loaded() call to happen in its own module or in the DOMContentLoaded event, after the ES modules have been loaded.

This change would need to be made in static/context.html and static/debug.html.

Current code:

  <script type="text/javascript">
    window.__karma__.loaded();
  </script>

Proposed change:

  <script type="text/javascript" nomodule>
    window.__karma__.loaded();
  </script>
  <script type="module">
    window.__karma__.loaded();
  </script>

Or alternatively, using the DOMContentLoaded event:

  <script type="text/javascript">
    document.addEventListener("DOMContentLoaded", function() {
      window.__karma__.loaded();
    });
  </script>

A similar change would need to be done by anyone who is using a custom context.html file.

I'm not sure if this would affect the static/client_with_context.html file as I don't know how the tests are kicked off in that scenario.

[appsforartists]

Thanks @Coridyn! (You might have noticed this is the first time I've PRed karma; not terribly familiar with its internals.)

I've made those changes. The tests are passing locally; hopefully in 20m, they'll pass on Travis too.

[appsforartists]

HOORAY! Travis passed. (Took more work than I expected, because apparently Chrome doesn't work on Karma's Travis setup, and Firefox doesn't enable modules by default.)

[lastmjs]

Anything I can help with to move this along? I could really use these changes

[johnjbarton]

@lastmjs you can figure out why the appveyor build fails.

[surma]

As a workaround for people like me who are waiting for this to land upstream: You can rip out static/context.html and static/debug.html from this PR and put it in your karma.conf.js

const configuration = {
  /* ... */
  customContextFile: 'tests/context.html',
  customDebugFile: 'tests/debug.html',
  /* ... */
};

[surma]

Another note: The provided context.html will just ignore test files that don’t load successfully instead of failing the test suite.

Example:

// module.test.js — loaded as a module
import * as Something from '/this-file-doesnt-exist.js';

describe('Something', function () {
  it('should do something', function () {
    // ...
  });
});

This test file should fail as the import does not exist. Instead Karma just ignores the file and succeeds.

[jehon]

I tried to fix this PR, made a checkout etc... but could not make it run.

So, because I am a bit impatient, I made a small plugin here: https://www.npmjs.com/package/karma-jasmine-es6. This plugin took me 30 minutes, so it is not really a waste of time.

This plugin is meant to be a temporary fix for a short time, while waiting for this PR to accomplish its mission.

Btw, thanks in advance to anybody who would manage to fix this PR !

[sergei-startsev]

@johnjbarton I've rebased the PR and checked the build status in another PR #3072, both builds are green, are there any other concerns to merge this one?

[vitalets]

Hi everyone! Thanks to this issue - I've got ES modules working for local files. But it throws error for imports from node_modules. For example:

// test.js
import uuid from 'uuid';

Outputs:

Chrome 68.0.3440 (Mac OS X 10.13.6) ERROR
  {
    "message": "Uncaught TypeError: Failed to resolve module specifier \"uuid\". Relative references must start with either \"/\", \"./\", or \"../\".\nat http://localhost:9876/context.html:0:0\n\nTypeError: Failed to resolve module specifier \"uuid\". Relative references must start with either \"/\", \"./\", or \"../\".",
    "str": "Uncaught TypeError: Failed to resolve module specifier \"uuid\". Relative references must start with either \"/\", \"./\", or \"../\".\nat http://localhost:9876/context.html:0:0\n\nTypeError: Failed to resolve module specifier \"uuid\". Relative references must start with either \"/\", \"./\", or \"../\"."
  }

How do you solve this?
Thanks!

[appsforartists]

@vitalets Node's module resolution is supported by Webpack, but not part of the ES2015 spec, which means browsers don't support it.

You can either make your imports into paths, e.g.:

import {} from './node_modules/some_library/dist/some_file.js';

or run your tests against a bundled source, e.g. from Webpack.

[johnjbarton]

Fixed by #3072