Feature extend config syntax Issue #278 #317

animous · 2013-01-22T04:19:28Z

Changed browsers configuration syntax as suggested in https://github.com/testacular/testacular/issues/278#issuecomment-11986383 except that I didn't know what to do with the headless: key proposed there. The list of args, if provided, completely replaces the default args normally used by that launcher. Within the arg list, occurrences of $TEMPDIR and $URL are replaced by the temporary dir and the URL passed in to browser.start, respectively. There is also a cmd: for specifying the path to the executable.

So browsers can now be:

browsers = [{
  name: 'Chrome',
  args: ['--user-data-dir=$TEMPDIR', '--start-maximized', '$URL'],
  cmd: '/usr/local/bin/chrome'
 }, {
  name: 'Firefox'
 }];

animous · 2013-01-22T15:45:27Z

Sorry, I fail at github. I was trying to add this as a pull request under issue #278. Not sure how it ended up as a separate thing.

bitwiseman · 2013-01-22T17:46:45Z

I'll jump in here.

Please read the Contributing.md - Initial setup instructions. They include details about commit message formatting (and how to automatically enforce it). You're going to need to redo these commits to match.

I also added some inline code review comments. I'm not the project owner take my feedback with a grain of salt.

Re github: It's cool. We're all beginners at some point. Thanks for contributing!

animous · 2013-01-22T18:28:57Z

Weird. Before committing I ran grunt init-dev-env and noted that it set up a .git/hooks/commit-msg. It didn't bark when I committed, but it looks like I'll have to read the commit message spec after all :). I can take a stab at fixing the commit messages, but I'm new to the pull request mechanics. Do folks typically git rebase -i and git push -f the same branch already submitted in the pull request or is there a better path?

bitwiseman · 2013-01-22T20:02:02Z

re init-dev-env: ... oh great. It works for me from the command line, but if I use "git gui" it doesn't. :(

Filed #319.

DavidSouther · 2013-01-22T22:25:54Z

animous#1

I added the normalization @bitwiseman talks about to maintain backwards compatibility, and I've reverted most of the changes to the tests. It also handles adding args, rather than replacing. I think that answers most of the design concerns @bitwiseman mentions.

animous · 2013-01-23T04:37:53Z

FWIW, I originally went for replacing the default options, rather than appending, because the Opera.js launcher looked like it might need all of the options to precede the URL. I didn't know enough about the various browsers to be sure that none of them would need the flexibility of more complete control over the args, but if the order of the arguments isn't important and if folks never need to remove any of the default args, then it's more convenient and probably hits most use cases if we simply append.

What's the next step? @DavidSouther sent a pull request that seems to address many of @bitwiseman's concerns (thanks!), but it looks like we both probably need to fix our commit log messages, perhaps scrap the variable substitution stuff, and add some more tests. Do we keep plowing ahead on this branch? Should we try to rebase this into a cleaner set of patches (e.g., eliminating changes in the tests that I shouldn't have changed) or is it better to close this and cherry pick changes onto a new branch? Or maybe wait for direction from maintainers? :)

bitwiseman · 2013-01-23T05:31:26Z

Arg ordering: you may be right about Opera ordering - it can still be additive, but the launcher can put config args before the url. The replace scenario is also worth doing, I just want to iterate to it.

I can help with the commit messages. You can wait to see if maintainers have an opinion, but if you're amenable, we're making prgress, I'd say "be bold" - branch off your changes for safekeeping, then let's roll forward.

dignifiedquire · 2013-01-23T19:46:55Z

lib/cli.js

@@ -40,7 +40,7 @@ var processArgs = function(argv, options) {
  }

  if (helper.isString(options.browsers)) {
-    options.browsers = options.browsers.split(',');
+    options.browsers = options.browsers.split(',').map(function(browser) { return { name: browser }; });


Please no one liners.

Would you prefer to see

options.browsers = options.browsers.split(',') .map(function(browser) { return { name: browser }; });

or

options.browsers = options.browsers.split(',') .map(function(browser) { return { name: browser }; });

None of them ;) I think

options.browsers = options.browsers.split(',').map(function(browser) { return { name: browser }; });

is the way to go. But at this point I want your version anyhow with the normalization so I think we can actually remove this and leave cli.js untouched.

Cool, I think I like that better as well.

dignifiedquire · 2013-01-23T19:53:58Z

Wow. First of all thanks to all who contributed to the PR and the discussion. I've added some comments inline for basic stuff. To answer and clarify some of the rest

This whole PR should be one commit message.
Just merge the stuff from @DavidSouther in. Backwards compatibility is a must.
I think it should add the cli args to the default ones, as most of the time one just wants to add one thing. Maybe we should add a boolean flag overrideArgs to change this behaviour.

DavidSouther · 2013-01-23T19:56:01Z

@dignifiedquire thanks for the commentary, I've addressed them in my branch.

Would it make most sense for @animous to rebase and squash all these commits?

dignifiedquire · 2013-01-23T20:00:17Z

@DavidSouther Thanks. Yes squashing is the way to go.

animous · 2013-01-24T03:31:32Z

I tried to address the comments above by rebasing and squashing.

It sounded like maybe I'm supposed to git push -f the rebased branch, but I try to be careful with -f and wasn't sure what github would make of this. So for now, I put the rebased squash commit on a separate branch (animous@bc9dd65). Please let me know what the right way is to amend the pull request. Thanks to everyone for all the help.

Update: @bitwiseman says git push -f will do the right thing, so I'm going for it.

DavidSouther · 2013-01-28T17:54:28Z

Any updates on this? Has the code been merged/will it be merged/if so, when?

dignifiedquire · 2013-01-28T18:52:29Z

test/e2e/coverageRequirejs/testacular.conf.js

@@ -13,7 +13,7 @@ exclude = [
 ];

 autoWatch = true;
-browsers = ['Chrome'];
+browsers = [{ name: 'Chrome' }];


Can we revert this back to ['Chrome']?

dignifiedquire · 2013-01-28T18:56:57Z

@animous I've added some small comments inline. One thing could you add a test for the config if it correctly normalizes the browsers? Otherwise I'd say it looks good to merge. What do you think @vojtajina?

@DavidSouther Sorry for the delay, still working on those docs..

Support optional `cmd` and `args` parameters in the `browsers` configuration. The `args` are appended to the default command-line options for the browser. If `overrideArgs` is true, then the `args` replace the default command-line options for the browser. Occurrences of `$TEMPDIR` or `$URL` within the `args` are replaced by the temp directory and URL. Use case is to pass special options, e.g., to load browser extensions. Closes karma-runner#278

animous · 2013-01-30T04:31:51Z

@dignifiedquire I reverted the changes to browsers under test/e2e as requested. Also reverted the change down around line 145 of test/unit/config.spec.coffee.

For the other comments on test/unit/config.spec.coffee, I copied @DavidSouther's config7.js test to config8.js and reverted the change to config7.js in hopes that it will be clearer that we're covering the "normalize strings" case as well as the "normalize string and object" case. (I probably could have replied inline above, but wasn't sure what would happen to those inline comments when I brazenly git push -f.)

I squashed the reverts and edits into the original commit, rebased onto the latest master, and did a git push -f (since that seems to have been okay last time).

dignifiedquire · 2013-01-30T06:56:56Z

@animous Thanks for all the good work. From my side it looks good. Will need to wait for @vojtajina to give its 👍

bitwiseman · 2013-01-30T20:34:24Z

A couple little nitpicks that got lost in the shuffle. Other than that, LGTM.

vojtajina · 2013-02-03T05:35:05Z

Guys, thanks everyone! This is really great, people even reviewing pull requests.

After a quick look, it looks good to me. I'm gonna merge it after the refactoring branch, because I already separated all the launchers into separate npm modules (plugins): https://github.com/vojtajina/testacular/tree/using-di

I will take care of merging this PR with the branch mentioned.

jeffastorey · 2013-03-26T04:23:08Z

Has this been merged in?

jordanpotter · 2013-04-03T01:30:15Z

I'm new to Karma, but I'm exceptionally excited about this change!

alindsay55661 · 2013-04-10T23:15:02Z

Really looking forward to this blocker biting the dust as well!

shadowedged · 2013-04-22T13:28:50Z

I'm waiting for this to be merged any day now.

karma.defineLauncher('my_chrome', 'Chrome', { flags: ['--start-maximized'] }); karma.definePreprocessor('coffee_bare', 'coffee', { options: { bare: true } }); karma.defineReporter('html_coverage', 'coverage', { ... }); The reason, why I decided for this solution, rather than inlining like this: browsers = [{ type: 'Chrome', flags: ['--start-maximized'] }]; Is that the solution above allows using these custom launchers from CLI. It also makes it easier to create only a single instance of a preprocessor (if the configuration is the same). Closes karma-runner#317

vojtajina · 2013-05-06T18:15:18Z

Guys, please check out the PR #533

dignifiedquire reviewed Jan 23, 2013
View reviewed changes

DavidSouther mentioned this pull request Jan 23, 2013

278 launcher animous/testacular#1

Merged

dignifiedquire reviewed Jan 28, 2013
View reviewed changes

dignifiedquire mentioned this pull request Mar 17, 2013

BrowserStack support #96

Closed

ghost assigned vojtajina Mar 19, 2013

vojtajina mentioned this pull request May 6, 2013

feat(config): allow configurable launchers, preprocessors, reporters #533

Merged

vojtajina closed this in 76bdac1 Jun 13, 2013

google-admin unassigned vojtajina Mar 26, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature extend config syntax Issue #278 #317

Feature extend config syntax Issue #278 #317

animous commented Jan 22, 2013

animous commented Jan 22, 2013

bitwiseman commented Jan 22, 2013

animous commented Jan 22, 2013

bitwiseman commented Jan 22, 2013

DavidSouther commented Jan 22, 2013

animous commented Jan 23, 2013

bitwiseman commented Jan 23, 2013

dignifiedquire Jan 23, 2013

DavidSouther Jan 23, 2013

dignifiedquire Jan 23, 2013

DavidSouther Jan 23, 2013

dignifiedquire commented Jan 23, 2013

DavidSouther commented Jan 23, 2013

dignifiedquire commented Jan 23, 2013

animous commented Jan 24, 2013

DavidSouther commented Jan 28, 2013

dignifiedquire Jan 28, 2013

dignifiedquire commented Jan 28, 2013

animous commented Jan 30, 2013

dignifiedquire commented Jan 30, 2013

bitwiseman commented Jan 30, 2013

vojtajina commented Feb 3, 2013

jeffastorey commented Mar 26, 2013

jordanpotter commented Apr 3, 2013

alindsay55661 commented Apr 10, 2013

shadowedged commented Apr 22, 2013

vojtajina commented May 6, 2013

Feature extend config syntax Issue #278 #317

Feature extend config syntax Issue #278 #317

Conversation

animous commented Jan 22, 2013

animous commented Jan 22, 2013

bitwiseman commented Jan 22, 2013

animous commented Jan 22, 2013

bitwiseman commented Jan 22, 2013

DavidSouther commented Jan 22, 2013

animous commented Jan 23, 2013

bitwiseman commented Jan 23, 2013

dignifiedquire Jan 23, 2013

Choose a reason for hiding this comment

DavidSouther Jan 23, 2013

Choose a reason for hiding this comment

dignifiedquire Jan 23, 2013

Choose a reason for hiding this comment

DavidSouther Jan 23, 2013

Choose a reason for hiding this comment

dignifiedquire commented Jan 23, 2013

DavidSouther commented Jan 23, 2013

dignifiedquire commented Jan 23, 2013

animous commented Jan 24, 2013

DavidSouther commented Jan 28, 2013

dignifiedquire Jan 28, 2013

Choose a reason for hiding this comment

dignifiedquire commented Jan 28, 2013

animous commented Jan 30, 2013

dignifiedquire commented Jan 30, 2013

bitwiseman commented Jan 30, 2013

vojtajina commented Feb 3, 2013

jeffastorey commented Mar 26, 2013

jordanpotter commented Apr 3, 2013

alindsay55661 commented Apr 10, 2013

shadowedged commented Apr 22, 2013

vojtajina commented May 6, 2013