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

karma run overwrites client.args state #1746

Closed
danielcompton opened this issue Dec 4, 2015 · 4 comments · May be fixed by Omrisnyk/npm-lockfiles#122 or Omrisnyk/npm-lockfiles#132
Closed

karma run overwrites client.args state #1746

danielcompton opened this issue Dec 4, 2015 · 4 comments · May be fixed by Omrisnyk/npm-lockfiles#122 or Omrisnyk/npm-lockfiles#132

Comments

@danielcompton
Copy link
Contributor

Summary

karma run calls this line lib/middleware/runner.js#L50 which overwrites the state of config.client.args with an empty array. This means that plugins that rely on config.client.args will run when calling karma start, but fail after calling karma run once (even when they are subsequently autorun from the karma start process).

Details

I'm using karma-cljs-test to run my ClojureScript tests. It needs a main function to be provided in config.client.args. I was able to run my tests successfully through the auto runner in karma start but when I called karma run, my tests would fail to run as config.client.args had been overwritten with an empty array. Because config is a mutable object, once I had called karma run once, my tests would fail when being rerun by the karma start autorunner.

Commenting out

config.client.args = data.args

let my tests pass, but this is clearly not a viable solution. One option would be to only set config.client.args if data.args is not an empty array.

This also raises the wider question of why a karma run is setting global mutable state. That behaviour was surprising to me, and I don't think it is intentional.

This behaviour seems to have been introduced in ca4c4d8, and relates to 27b8d67, #283, karma-runner/grunt-karma#79, karma-runner/grunt-karma#79, karma-runner/grunt-karma@b658ec9 and karma-runner/grunt-karma@168f7b7.

danielcompton added a commit to danielcompton/karma that referenced this issue Dec 9, 2015
danielcompton added a commit to danielcompton/karma that referenced this issue Dec 9, 2015
danielcompton added a commit to danielcompton/karma that referenced this issue Jan 11, 2016
@xtianjohns
Copy link

Some introspection into whether or not the objects share properties would be really great.

I'm experiencing a pretty crummy situation right now with karma-browserstack-launcher that involves parameterizing Browserstack access keys.

karma.conf.js:

//...
browserStack: {
  'startTunnel': true,
  'project': 'some_project_name',
  'build': 'some_build_name',
  'rateLimitTimeout': 5000
},
//...

This command, run by Jenkins or some other CI system that wants to pass username/access tokens would look like:

karma start --browserstack.username=my_username --browserstack.accessKey=some_key

Executing this will essentially throw away any configuration that would have been made in karma.conf.js.

The workaround in my situation is to have the CI pass all of the arguments. And then I lose all of the flexibility of having the keys in a configuration file for developers to share.

I'm not sure whether your situation, @danielcompton, is unique to karma run or not, I've only experienced my behavior with karma start and haven't tried karma run.

@danielcompton
Copy link
Contributor Author

This sounds like a similar problem of handling input from the command line, although the exact mechanics to fix it will be different.

@dignifiedquire
Copy link
Member

Thanks @danielcompton for the detailed write up. From looking at this I feel the most sensible option would be to merge the two together (with the options provided from run taking precedence) rather than replacing them. Do you think that would be a good solution?

@danielcompton
Copy link
Contributor Author

I feel the most sensible option would be to merge the two together

Yep, I think you're right, that was the conclusion I came to. It could also be good to make the Karma config created at karma start immutable, and have karma run make a copy of the config. However that sounds like a much bigger change :)

danielcompton added a commit to danielcompton/karma that referenced this issue Feb 26, 2016
Before this change, calling `karma run` would overwrite any value in
config.client.args with the value provided in the `karma run` request,
even if that value was an empty array. This commit does a _.merge to
merge the two values together.

Fixes karma-runner#1746
danielcompton added a commit to danielcompton/karma that referenced this issue Feb 29, 2016
Before this change, calling `karma run` would overwrite any value in
config.client.args with the value provided in the `karma run` request,
even if that value was an empty array. This commit does a _.merge to
merge the two values together.

Fixes karma-runner#1746
danielcompton added a commit to danielcompton/karma that referenced this issue Feb 29, 2016
Before this change, calling `karma run` would overwrite any value in
config.client.args with the value provided in the `karma run` request,
even if that value was an empty array. This commit does a _.merge to
merge the two values together.

Fixes karma-runner#1746
danielcompton added a commit to danielcompton/karma that referenced this issue Jun 20, 2016
Before this change, calling `karma run` would overwrite any value in
config.client.args with the value provided in the `karma run` request,
even if that value was an empty array. This commit does a _.merge to
merge the two values together.

Fixes karma-runner#1746
danielcompton added a commit to danielcompton/karma that referenced this issue Jun 20, 2016
Before this change, calling `karma run` would overwrite any value in
config.client.args with the value provided in the `karma run` request,
even if that value was an empty array. This commit does a _.merge to
merge the two values together.

Fixes karma-runner#1746
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants