search

rolaveric / karma-systemjs

Karma plugin for using SystemJS as a module loader
MIT License
40 stars 19 forks source link

testFileSuffix runs tests in dependencies #38

Closed nicklasb closed 9 years ago

nicklasb commented 9 years ago

Hi,

I am trying to get testing going in my project and I am encountering a problem where I need to make karma publish my dependencies to make them available for the testing to work like this: {pattern: 'jspm_packages/**/*.*', included: false, served: true}

The problem is that if I set the testFileSuffix to "spec.js", it tries to run all the tests in the dependencies as well, like angulars' tests. To make them not run, I have to narrow down the suffix to something that only matches my tests.

Is this intended behaviour, I am thinking that this is likely related to karma-systemjs as it is a karma-systemjs configuration option?

rolaveric commented 9 years ago

One option you can try is to use testFileRegex instead of testFileSuffix to be more specific (I should really add doco to the README).

Another option is you can use exclude in the Karma config. You give it globs to exclude completely from Karma: http://karma-runner.github.io/0.12/config/configuration-file.html

nicklasb commented 9 years ago

So this is intended behaviour? Is there no way in using the included option from the pattern to automagically exclude those things? Something in karma must make that distinction?

rolaveric commented 9 years ago

No, it shouldn't work any differently. The jspm_packages folder isn't treated any differently by karma or karma-systemjs. On 13 Aug 2015 22:04, "Nicklas Börjesson" notifications@github.com wrote:

So this is intended behaviour? Is there no way in using the included option from the pattern to automagically exclude those things? Something in karma must make that distinction?

— Reply to this email directly or view it on GitHub https://github.com/rolaveric/karma-systemjs/issues/38#issuecomment-130644064 .

nicklasb commented 9 years ago

I am not sure I understand, doesn't the included options tell karma not to load the files, just to serve them? What seems strange is that they still seems to be loaded.

rolaveric commented 9 years ago

Sorry, you're right - it is different from using vanilla Karma. What karma-systemjs does is get the list of all files that Karma is serving, looks for the ones it thinks are tests (using testFileRegex or testFileSuffix) and calls System.import(filePath) on them.

nicklasb commented 9 years ago

Hm, is there any way to instead build that list from the settings, perhaps using karmas' own code for that? It has to do that somewhere, right?

Because there are a couple of things that aren't great:

  1. It loads, if it is a large application with many dependencies, a boatload of unnecessary files.
  2. Makes testFileSuffix et al works in a unexpected fashion, different from vanilla Karma.
  3. The developer has to always invent a reliably unique suffix that matches ones own test only and name the test file to dfi4wj.spec.js (and explain why it has that strange name in the documentation)
rolaveric commented 9 years ago

I don't necessarily agree with the first point. Assuming it can accurately identify the correct test suites, then it should only 'load' the minimum files necessary to run those tests.

However there's clearly a problem with that assumption - karma-systemjs is picking more files than you want, and the configs mechanisms available aren't adequate to fix that.

So what's the best solution? Here's the options that I can think of:

  1. Run System.import() for every file that would normally have a {included: true} pattern in standard Karma. This should be ok. Even if you import things twice (eg. Because it's imported by another file) it'll be cached anyway - so no performance drawback. There could be side effects from loading modules, but that's always been a risk, really. My only hesitation is that it's very different from how karma-systemjs is configured today, but this wouldn't be the first or last open source project to change APIs.
  2. Introduce better controls for specifying the tests you actually want to run. eg. globs ("src/*/.test.js"), specifying a sourceFolder that then gets used as a prefix for test suite paths, etc. The glob is probably the best option.

What do you think?

nicklasb commented 9 years ago

I would prefer 1, simply because it, in my opinion, is the correct one. It is either that, or having to answer this question indefinitely. And those who rely on it working like this has a really strange configuration that at least I would not like to support. Having plugins diverging radically from how parent projects works confuses uses and drives issues to the plugin project.

Breaking change is what major releases are for. I wouldn't be too afraid by them, especially when the plugin is this young. I mean, I know that there are users but here hasn't even been a release yet, I suppose that is intentional, but it should signal to users that this is a very young project.

nicklasb commented 9 years ago

Edited the above comment significantly (if you are reading this via mail).

rolaveric commented 9 years ago

OK, I've done a first crack at it. Committed under 0.8 branch, and published to npm as '0.8.0-beta2': https://github.com/rolaveric/karma-systemjs/tree/0.8

Updated my angular-seed repo to use it too: https://github.com/rolaveric/angular-seed/tree/es6

So what it does now is it takes every file pattern with {included: true}, uses minimatch to generate a RegExp string from the glob pattern that can be passed to the browser, then sets {included: false} so Karma doesn't add a Githubissues.

  • Githubissues is a development platform for aggregating issues.