feat: pass command line opts through to browser

[timbertson]

For large test suites, always running all the tests is slow. This change allows command-line arguments to be passed through to the runner as the single option to __karma__.run. This allows you to target specific tests per-run (when combined with a browser-side runner function that uses this feature).

BREAKING CHANGE:

The config file is no longer assumed to be the first argument, it must be specified with either -c or --config-file. I had to do this in order for unknown arguments to be pass through unmolested.

[timbertson]

The travis build is failing because the e2e test config file can't be found. I tried to fix the grunt task for this, but I've never used grunt before and can't figure it out.

[vojtajina]

This is why we have iit and ddescribe.

[vojtajina]

Or with mocha, it.only, describe.only

[timbertson]

I know it depends on your test runner, but the workaround you describe covers far fewer cases than passing arbitrary arguments. For example:

• running a specific folder of test modules / files, e.g (karma run -- tests/unit/) (impossible to do with ddescribe, since there's no single scope that represents this)
• running non-contiguous tests baed on keyword (karma run -- :IE)
• affecting things other than test running (karma run -- --no-logcapture --bail)

These are all examples that work for the Oni Apollo test runner, which is what I'm using. I'm not expecting specific support or anything because it's obviously not one of the common platforms.

But please, could there be some way to get some state / config passed from the running terminal to the browser running the tests? It doesn't have to be this patch if you don't like the changes I've make to the command line arguments, but without this feature (or something like it) you have to rely solely on what is present on on-disk files, which greatly limits interactive use on the terminal.

[vojtajina]

this won't work for bigger data, it's a stream - you can receive the data in multiple events, you need to use json parser that can handle streams, or buffer it

[vojtajina]

Well, the main use case for Karma is to stay in the editor and run the tests on save, using auto-watch (also using auto-watch is more efficient than karma run).

That said, I'm not against passing arguments into the browser.

[vojtajina]

@gfxmonk I'm actually getting warmed on this... ;-)

Why can't you keep the config file arg as it was ?

[timbertson]

Good to hear :)

Assuming that the config was the first arg means that you must always provide the config as the first arg. Which is unnecessary when you're running karma start and are using the default config location, but downright confusing when using e.g karma run test/unit (it parses "test/unit" as options.configFile despite not actually using it).

Although looking at this again, it seems that the karma run arg-parsing behaviour is actually buggy, and doesn't need to be kept - i.e. cli.js should not take the first argument as configFile when the action is run. Is that true?

In which case, I can rework the branch to leave configFile as the first argument for start and init commands, but remove it for run.

[vojtajina]

Yep, at this point, the only option used for runner is the --runer-port,
but I think having all commands to accept config file as the first argument
makes it simple. (One could point karma run to a config file to read the
runnerPort, rather than specifying the port.

Anyway, even if we remove the config from karma run, everything that goes
to the browser should be explicit, eg. separated by -- or in a separate
flag --client-config or something.

There was also a discussion about exposing the whole config into the
browser.

Maybe we could add "client" config, that would be passed to client.

// in karma.conf.js
client = {
  autoStart: false, // require.js etc. need to explicitly kick off the run,
  captureConsole: true
};

From cli, you could do karma start --client.autoStart false, karma run --client.filter :IE...

Don't know, just throwing some ideas...

On Sat, May 11, 2013 at 9:18 PM, Tim Cuthbertson
notifications@github.comwrote:

Good to hear :)

Assuming that the config was the first arg means that you must always
provide the config as the first arg. Which is unnecessary when you're
running karma start and are using the default config location, but
downright confusing when using e.g karma run test/unit (it parses
"test/unit" as options.configFile despite not actually using it).

Although looking at this again, it seems that the karma run arg-parsing
behaviour is actually buggy, and doesn't need to be kept - i.e. cli.js
should not take the first argument as configFile when the action is run.
Is that true?

In which case, I can rework the branch to leave configFile as the first
argument for start and init commands, but remove it for run.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/530#issuecomment-17772278
.

[timbertson]

But if you make the first argument to each command be the config, it means you have to pass an (unused and unnecessary) config argument to karma run - even passing karma run -- arg will treat arg as the configFile option with the current implementation.

Aside from that, I agree that it should be explicit regardless of how args are parsed. So I've pushed a new version (to the same branch; should I be creating a new PR when I do this instead? Not sure of the etiquette here). It stops parsing at any '--' arg, and passes everything past that as "clientArgs", an array of strings. So there are no longer changes to existing use cases (i.e the config argument).

I added tests in a separate commit, as I'm not sure whether the feature is important enough for a whole new e2e test. But it was useful for me, so I've added it anyway. Feel free to just take the first commit if you don't think it's worth its own e2e test.

[geddski]

I needed this as well, and solved it a bit differently in #574. I just used underscore's "defaults" function to add any non-karma config properties back onto the config, then make sure the full config object was being sent to the client. I guess you could have collisions though if karma used the same property the adapter was looking for.

[timbertson]

I'd advise against passing a config object, as that forces optimist's
interpretation of command line arguments onto test runners, making it
difficult for karma to switch to another argument parser in the future. My
test runner uses dashdash, so if I got an optimist options object I'd have
to try and reverse-engineer an array of arguments from that, which might
not even be unambiguous (especially if it's merged with karma options).

[vojtajina]

The reason why I closed #574 and moved the discussion here is that it didn't address passing arguments from karma run and I believe that's the most important use case (passing arguments only with karma start can be pretty much done with just having the client config in a JS file and including that file).

Can we list all the use cases for this feature ?

I'm thinking about having "client" object in the config and that's what gets passed into the client. That means from the CLI you would do karma run --client.grep whatever...

[timbertson]

My use case is a test runner that works in nodejs and the browser. In nodejs, it parses command line arguments from process.argv. In the browser, it parses them from document.location.hash (using a port of https://github.com/substack/node-shell-quote to break up the string into an array of arguments consistent with the command line).

I've added some minimal code in my test runner's karma adapter to inject the arguments array directly (as if they came from process.argv). But this only works with my fork, where the config passed by karma has an array of unmodified command line argument strings.

I have test wrapper that looks a bit like:

node test/suite.sjs "$@"
karma run karma/conf.js -- test/suite.html "$@"

So to run a certain test or set of tests across all platforms (node and the browser), I'll run e.g: ./test.sh unit/sequence-tests.sjs:map

So it's important (to me) that the options to node and via karma end up looking exactly the same to my test runner. Having a custom options object, or prefixed keys (--client.grep), would not be acceptable - I could no longer run the same tests in nodejs and karma using the same set of arguments.

It seems to me that the lowest common denominator is argument lists - that's what all node-based runners will already be using. Making them deal with a different format makes it harder for them to integrate with karma. Having some other format (a pre-cooked options object, or prefixed options) is OK for runners that only work in the browser, but it's going to be frustrating for everyone who uses a runner that works in both node and via karma, because you're always going to have to write different arguments (despite the underlying options you're converying to the runner being identical).

Aside from node.js compatibility, I don't like optimist's argument parsing. Karma can't know what options my runner expects in advance, so when it sees --bail unit/sequence-tests.sjs it can't possibly know whether that means {bail: "unit/"} or {bail: true, _args: ["unit/"] }. Optimist is probably fine for karma's use of command line arguments, but I don't think it's good enough to be forced on runners.

[vojtajina]

Also adding ref to related issues: #380 #242

[vojtajina]

I'm not tight to optimist - if there's a better arg parser, I'm fine switching to it. Optimist was the best tool I found at that time. That said, it currently works fine, and there are other priorities ;-)

@gfxmonk your node runner is not a typical use for Karma, I don't expect people to be using other runners with Karma. Should I ? What is the benefit ?

So far I can see only one real use case for "passing args to the client" and that is filtering the tests, eg. #242 (comment)

Other client config seems to be static (eg. captureConsole, autoStart) - you would define it in the karma.conf.js. I'm fine supporting that too, however including a JS file with it is pretty much the same thing.

At this point I'm leaning towards karma run --client.filter aaa --client.other whatever and client in the karma.conf.js too (for stuff like autoStart, captureConsole.

[vojtajina]

@gfxmonk that means, you would have to transform the args for your runner, however I don't think it's that big of deal. You already have a wrapper script anyway, so you can call this wrapper script with what you want: ./test.sh unit/sequence-tests.sjs:map and inside transform the unit/sequence-tests.js:map into --client.filter unit/sequence-tests.js Would that work for you ?

Also, I'm not sure, how you wanna filter the tests in the browser, using "unit/sequence-tests.js", unless you are using Require.js or something like that.

[geddski]

client.x looks good to me.
On Jul 3, 2013 9:52 PM, "Vojta Jina" notifications@github.com wrote:

I'm not tight to optimist - if there's a better arg parser, I'm fine
switching to it. Optimist was the best tool I found at that time. That
said, it currently works fine, and there are other priorities...

@gfxmonk https://github.com/gfxmonk your node runner is not a typical
use for Karma, I don't expect people to be using other runners with Karma.
Should I ? What is the benefit ?

So far I can see only one real use case for "passing args to the client"
and that is filtering the tests, eg. #242#242 (comment)

Other client config seems to be static (eg. captureConsole, autoStart) -
you would define it in the karma.conf.js. I'm fine supporting that too,
however including a JS file with it is pretty much the same thing.

At this point I'm leaning to karma run --client.filter aaa --client.other
whatever and client in the karma.conf.js too (for stuff like autoStart,
captureConsole.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/530#issuecomment-20457894
.

[timbertson]

@gfxmonk your node runner is not a typical use for Karma, I don't expect people to be using other runners with Karma. Should I ? What is the benefit ?

I don't understand. Karma is not itself a test framework, so wouldn't most people be using "other runners"? My "runner" is not conceptually any different to mocha. Given that mocha takes arguments on the command line when running in nodejs, it seems entirely reasonable that it would want to do the same thing when running under karma. For example, if the mocha karma adapter were to support it, you might run:

$ mocha --grep=myTest test1.js
$ karma run karma.conf -- --grep=myTest test1.js

To run the same tests in both node and the browser. requiring all options to be prefix with --client doesn't even make sense when you have non-option arguments (test1.js in this case).

You already have a wrapper script anyway, so you can call this wrapper script with what you want [... ] Would that work for you ?

I don't actually want to have a wrapper, so forcing a solution that requires pre-processing of options isn't actually a good thing. I haven't looked into it yet, but I'd ideally like to get rid of my wrapper and just become an adapter / karma plugin.

Also, I'm not sure, how you wanna filter the tests in the browser, using "unit/sequence-tests.js", unless you are using Require.js or something like that.

Yeah, it's a similar result to requireJS. Feel free to pretend that "unit/sequence-tests.js" is just an arbitrary string used to filter tests based on their description - the fact that it happens to be a file path isn't really important.

There are (at least) two problems with prefixing everything with --client-:

• it provides no way to pass "non-option" arguments? i.e most test runners accept a list of filenames after any options - how do you plan to pass those?
• it becomes impossible to run with the same arguments for nodejs and karma (I must use --foo for one and --client-foo for the other). This is not much of a problem for interactive use, but it then makes it impossible to run a single script that runs both nodejs and karma with the same options. You could get people to write adapters to "translate" one into the other, but why would you want to do that?

Additionally, having the options pre-parsed into an object (rather than an array of strings) without knowing what options are expected is fundamentally intractable. Optimist is forced to make guesses because of this. For karma, those guesses happen to be good enough because it has very few options. For my runner (and for mocha) they are not. I gave an example of this in my last comment (--bail).

I feel like I've (already) listed a number of reasons for not wanting everything to be prefixed with --client-. And yet you still seem to be keen on the idea. Can I ask why? Are there good reasons for not wanting to simply pass everything after a "--" as an array? What benefit do we get by intentionally being less general?

If you insist on prefixing client args with --client, then can we also pass all arguments after -- as client.argv (an unprocessed array of strings)? That would be perfectly acceptable to me.

[vojtajina]

What is the benefit of running the tests with Mocha and Karma at the same time ?

[timbertson]

As in, running the same tests under both?

To test code that is intended to run in a browser and in node.js. Perhaps app developers maintain two distinct test suites, but cross-platform library authors are sure to want to test the same code in all environments.

[geddski]

@vojtajina so we good for client.x soonish? Would like to start grepping like a mad man.

[timbertson]

I'm still waiting for a response to the multiple problems I've raised with the client.x strategy, or even a single benefit of it over my original proposed strategy (and patch) of just sending an array of additional argument strings.

[vojtajina]

• non-option argument is not a problem, you need to pass it into the browser somehow anyway, maybe the config object passed to browser can be {grep: 'xxx', _: ['one.js', 'two.js'], ...}; in the same way it can be done in the CLI (--client._ one.js, two.js
• running on nodejs/karma with the same options; again you either have your current wrapper, that do it, or if you start this other runner from within JS (as you said you wanna only start karma and the other runner would start from within a plugin) - doing this in JS is even simpler.

I'm scared of taking the whole space of everything after -- to be taken by client args, which really will only be used for filtering tests. But, maybe it makes sense for karma run. So how about this:

• there's a config object (in karma.conf.js) client, which has stuff like autoStart, captureConsole, etc...
• that implies, if we allow these to be set from the CLI, it would be karma start --client.capture-console false
• karma run would take everything after -- and pass it as client config, eg karma run -- --capture-console true one.js two.js becomes autoWatch: true, port: x, client: {_: ['one.js', 'two.js'], captureConsole: true}} and we ship only config.client to the browser

I think we should expose the config in the same way as the MAPPINGS - inline into the context.html, so that any file can access it immediately, rather than passing it as an argument to __karma__.start.

I'd like to see input from more people, as this still sound very specific to @gfxmonk use case (no offense dude, I really appreciate all your contribution to this project and I do wanna solve this).

[timbertson]

Thanks for the points, Vojta. I understand that my use case is probably unusual, but it's frustrating to have something conceptually simple ("pass command line arguments") be made unusable by applying a lossy algorithm in front of it.

I'm scared of taking the whole space of everything after -- to be taken by client args, which really will only be used for filtering tests.

This is a fair concern. Although I disagree that it's only going to be used for filtering tests. There are plenty of other behaviours of a test runner that you might want to affect for a single test run during development.

Maybe it makes sense for karma run. So how about this:
[ ... ]

I'm OK with your suggestion, except for the last point.

You still haven't addressed my point that parsing arguments before you know the available options cannot be done in the general case.

So I think while it makes sense to have (as you suggest):

$ karma run --client.capture-console false

send { "capture-console":false }

The only valid treatment of:

$ karma run -- --capture-console false

Is to send { _:["--client-capature", "false"]}

Because pre-processing will always be a guess, and it will often be wrong (this is why I refer to it above as applying a lossy algorithm). Here are a bunch of examples off the top of my head where it will be wrong:

• --filter false would send {filter: false}. You're assuming "false" == false, which is wrong.
• --bail test1 would send {bail: "test1"}. You're assuming that --bail takes an argument, which may be wrong. As a workaround, you could forbid flags and make every option require an argument, i.e --bail false. This makes the options accepted by karma incompatible with the options accepted by command line tools.
• -xyz test1. Does this send {x: true, y:true, z:true, _:["test1"]}, or {x: yz, _:["test1"]}, or {x: true, y:true, z:"test1"}
• -vvv or -v -v -v. How do you even parse this? {v:true}? {v:3}? {v: [null, null, null]}? {v: "vv"}. All are potentially valid interpretations, and which one is correct will depend on the application logic, which you can't possibly know.

So sure, you can make a convenience API that --client.x arg gets transformed into {x: "arg"}. But please leave arguments after -- as an unprocessed array of strings, because if you process them in any way, they will be wrong. It's simply not possible to get this right without intimate knowledge of the application logic.

[vojtajina]

That means your client code has to parse the arguments, but I think that makes sense.

Can you please:

• add a unit test for the cli parsing
• It should only parse the args after -- for karma run, no other commands.
• change the e2e test to use karma run -- arg1 arg2
• karma client should publish the config before reloading the iframe (as karma.config)
• only sent the client config to the browser

Sorry for delaying this PR so much @gfxmonk

[timbertson]

Thanks for coming around, @vojtajina :)

I've rebased this PR / branch and addressed all of those checklist items. Making the e2e test run karma serve && karma run was a bit awkward, as there's no way I could see to tell when the server is ready - so it just sleeps for 5s.

A minor concern is that karma.config seems like a too-general name - i.e. it would clash if we later decided to pass the contents of karma.conf.js as karma.config (as in #625 or #242). But if that doesn't bother you, this should be ready to merge :)

[vojtajina]

now we don't need this change at all, please revert

[vojtajina]

@gfxmonk great, don't worry about the rebasing, I will take care of it.

I don't know why Travis is failing, but it's probably because of the secured env vars don't work for pull requests, don't worry about that.

[timbertson]

Nits addressed in cli.js. Yeah having secured vars available to PRs would probably be a bad idea. Thanks for taking care of the rebase.

[vojtajina]

All right, I fixed some style things to make jshint/coffeelint happy and it's merged as 00d63d0

Thanks a lot @gfxmonk !!! Really appreciate your work !!!

[geddski]

Yay! Any chance we could we get some docs updates to show how to use this? Also wondering how to use it programmatically w server.start and runner.run, which is what we use in grunt-karma.

For the CLI I tried karma start karma.conf.js -- --grep and config.args is an empty array no matter what I try. Also it seems to fail completely when trying to pass options without the config file, like so:

karma start -- --grep --> ERROR [config]: File /Users/dave/create/grunt-karma/--grep does not exist!

[timbertson]

@geddski at @vojtajina's request, command line args are only allowed for the run command, not start.

So usage is:

$ karma start &
$ karma run -- --grep [...]

This is documented in the --help text for karma, although it's missing from karma run (fixed in #671)

As for runner.run, you would use:

runner.run( { clientArgs: ["--grep", "..."], ... }, cb);

I'm afraid I'm not familiar enough with karma's docs to know where that should be documented though.

[vojtajina]

You can define client config in karma.conf.js too:

module.exports = function() {
  config.set({
    client: {
      args: ['--foo', 'bar'],
      autoStart: false,
      captureConsole: false
    }
  });
};

[geddski]

Ah, nice. Thanks fellas.
On Aug 2, 2013 8:25 PM, "Vojta Jina" notifications@github.com wrote:

You can define client config in karma.conf.js too:

module.exports = function() {
config.set({
client: {
args: ['--foo', 'bar'],
autoStart: false,
captureConsole: false
}
});};

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/530#issuecomment-22046892
.

[geddski]

@vojtajina ok so with this, when passing the grep pattern, the Mocha adapter will receive clientArgs: [ '--grep=/whatever' ] instead of a parsed object. How would you recommend parsing this in the browser? Can't use optimist or other node tools at that point..

[geddski]

perhaps we could have a raw clientArgs and a parsed parsedClientArgs? Doesn't seem to make sense to force all the adapters to parse the args themselves.

[timbertson]

@geddski I'm using https://github.com/trentm/node-dashdash with minimal changes (for potentially missing Array.prototype.map / forEach), which works fine if you've got something requirejs-like. It shouldn't be too much work to make it work without requirejs either, as it has only one dependency.

[geddski]

Just realized I can parse the args in grunt-karma and pass them that way.

[maksimr]

Hi

I can not understand how it work :(

I start server

./node_modules/karma/bin/karma start --single-run=false

and then type grep

./node_modules/karma/bin/karma run --port 9002 -- --grep messages-item

But karma execute all tests... what I'm doing wrong?

I have karma 0.11.13 and karma-mocha 0.1.1

[maksimr]

Now adapter get settings in array, like this

__karma__.config.args = ['--grep test'];

And karma-mocha not allow grep in latest version of karma

[vojtajina]

@maksimr sounds like a bug in https://github.com/karma-runner/karma-mocha. Would you like to send a PR?

[geddski]

@maksimr maybe try without the extra --.

./node_modules/karma/bin/karma run --port 9002 --grep messages-item

[maksimr]

@geddski thanks but it doesn't help

[vojtajina]

karma run -- --grep is correct imho, but does anybody actually use this feature? I'm not sure if the adapter pass the args correctly to mocha (in a form that mocha expects).

@maksimr try to debug https://github.com/karma-runner/karma-mocha/blob/45553a286a9481ce01950b37e396eeb3ef66ca51/src/adapter.js#L83-L85 a bit, sorry I don't use mocha

[vojtajina]

@maksimr ignore my last message, just saw your PR....