-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
fix(runner): Merge config.client.args with client.args provided by run #1934
Conversation
@@ -166,6 +166,28 @@ describe('middleware.runner', () => { | |||
request.emit('end') | |||
}) | |||
|
|||
it('calling run with no client args should not overwrite existing client.args', (done) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test for the actual merge with different values please?
23687cc
to
10afa71
Compare
I've added tests for the following cases:
The only wrinkle here, and I'm not sure what can be done about it, is if the user passes |
10afa71
to
e715839
Compare
@danielcompton, How about checking the type of the client-args and take appropriate measures? |
What is the expected behaviour for merging an array and a map, and for merging a map and an array? |
God question, maybe just let the run args override the existing? On Mon, Feb 29, 2016, 09:17 Daniel Compton notifications@github.com wrote:
|
Yes I think in the case that the the types are not mergeable, i.e. array < object or object < array, we should override and print a warning. |
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
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
CLAs look good, thanks! |
This is ready for review now. |
Thanks |
Thank you! |
Before this change, calling
karma run
would overwrite any value in config.client.args with the value provided in thekarma run
request, even if that value was an empty array. This commit does a _.merge to merge the two values together.Fixes #1746