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

Delay __karma__.start() until es6 test modules have been loaded #3147

Closed
dwt opened this issue Sep 28, 2018 · 11 comments
Closed

Delay __karma__.start() until es6 test modules have been loaded #3147

dwt opened this issue Sep 28, 2018 · 11 comments

Comments

@dwt
Copy link

dwt commented Sep 28, 2018

Expected behaviour

It should be possible to use native es6 modules to specify unit tests

Actual behaviour

It currently does not work with the jasmine plugin

Environment Details

Karma version: 3.0.0

module.exports = function(config) {
  config.set({
    basePath: '.',
    frameworks: ['jasmine'],
    files: [
      { pattern: 'static/vendor/**/*.js', watched: false },
      { pattern: 'static/*.js', type: 'module', included: false },
      { pattern: 'static/tests/*.js', type: 'module' },
    ],
    exclude: [
    ],
    preprocessors: {
    },
    reporters: ['progress'],
    port: 9876,
    colors: true,
    logLevel: config.LOG_INFO,
    autoWatch: true,
    browsers: [],
    singleRun: false,
    concurrency: Infinity
  })
}

Steps to reproduce the behaviour

  1. Add a native es6 module to contain the jasmine unit tests
  2. Try to execute them
  3. Watch in awe as it doesn't work

If unit tests are loaded from modules (which is supported via the files configuration option) then __karma__.start() should really be delayed until after all the modules have been loaded, to give unit tests the ability to load before the start event.

I'm not sure about all test frameworks, but at least for jasmine it is necessary, as you cannot add tests to it, after it has started executing all it's configured tests.

So with a configuration like this:

    files: [
      { pattern: 'static/vendor/**/*.js', watched: false },
      { pattern: 'static/*.js', type: 'module', included: false },
      { pattern: 'static/tests/*.js', type: 'module' },
    ],

I would really like the context.html to read

  <script type="module">
    window.__karma__.loaded();
  </script>
  <script nomodule type="text/javascript">
    alert("your browser doesn't support modules, use a different one")
  </script>

instead of

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

which it is currently executing.

Not quite sure what the best way to patch this in would be, but it seems to work great for me in local tests.

So: are there any downsides to this approach? How would you be willing to take this as a pull request? It seems to make sense to me to actually delay start until after modules have loaded if you are using modules at all. Also erroring in browsers that don't support modules if you are telling karma that modules are to be uses seems to make a lot of sense. Am I missing something?

@dwt
Copy link
Author

dwt commented Sep 28, 2018

Accidentally first reported in the karma-mocha plugin karma-runner/karma-mocha#207

@dwt
Copy link
Author

dwt commented Oct 11, 2018

@maintainers: ping - can you guys reproduce this bug? Is what I'm proposing a valid approach to solving this? Is there a better one?

@johnjbarton
Copy link
Contributor

would you be willing to take this as a pull request?

Yes, assuming that the existing non-module use-cases continue to work.

I'm not an expert on es6 modules, but I guess the correct code might be more like

<!-- Delay loaded call until all modules are loaded, or noop if modules are not supported. -->
 <script type="module">
    window.__karma__.loaded();
  </script>
<!-- In browsers without module support, just load --> 
  <script nomodule defer type="text/javascript">
   window.__karma__.loaded();
  </script>

Base on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script

@dwt
Copy link
Author

dwt commented Oct 14, 2018

@johnjbarton This is something that you guys need to figure out. Of course one can build the fallback solution that calls window.karma.loaded() whenever modules are not supported int he current browser.

However I refrained from that approach because a test suite that does require to be run after modules have loaded, will break in this scenario (as does mine). So better to have fail fast. What I'm envisioning is that there are actually two templates. First the existing one, which is used whenever modules are not part of the sources that karma loads. Then a second one (the one I proposed) for when modules are used.

Making the test suite just magically start later on browsers that support modules (as you seem to propose) will make it much harder to debug why the test suite doesn't work on some browsers (but those browsers will of course get fewer and fewer over the years).

Either way - it's your project, so you will have to decide how you want to handle this - I'm happy to provide a pull request either way, because I think that it's important to support modules all the way through the stack without having to precompile all the JS.

So, how do you want it?

@johnjbarton
Copy link
Contributor

The designers of the module feature have a long history of backwards compatible solutions when new features are introduced. Unless you have specific information that their design fails, then I suggest we use it. It's not 'magical', is just how the feature is supposed to work.

That said, I do not have personal experience with the feature so the details will need to be checked.

@dwt
Copy link
Author

dwt commented Oct 15, 2018

@johnjbarton What details do you want to check, and who should do that? I'd love to get this patch in, but I don't really want to wait for weeks in standby to see what there is eventually to do.

@johnjbarton
Copy link
Contributor

Send a Pull Request if this is important to you.

@johnjbarton
Copy link
Contributor

Another solution to start-after-load is to register the call to __karma__.loaded() to a load event via addEventListener() and friend. This approach avoids any issue with browser implementations of nomodule.

@johnjbarton
Copy link
Contributor

Fixed by #3072

@dwt
Copy link
Author

dwt commented Oct 17, 2018

@johnjbarton thanks! Will test this tomorrow.

@dwt
Copy link
Author

dwt commented Oct 18, 2018

Works fine for me - thanks!

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

2 participants