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

Feature extend config syntax Issue #278 #317

Closed

Conversation

animous
Copy link

@animous animous commented Jan 22, 2013

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
Copy link
Author

animous commented Jan 22, 2013

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
Copy link
Contributor

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
Copy link
Author

animous commented Jan 22, 2013

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

animous commented Jan 23, 2013

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
Copy link
Contributor

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.

@@ -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 }; });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please no one liners.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 };
    });

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I think I like that better as well.

@dignifiedquire
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

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

@animous
Copy link
Author

animous commented Jan 24, 2013

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
Copy link
Contributor

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

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

autoWatch = true;
browsers = ['Chrome'];
browsers = [{ name: 'Chrome' }];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dignifiedquire
Copy link
Member

@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
Copy link
Author

animous commented Jan 30, 2013

@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
Copy link
Member

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

@bitwiseman
Copy link
Contributor

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

@vojtajina
Copy link
Contributor

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.

@ghost ghost assigned vojtajina Mar 19, 2013
@jeffastorey
Copy link

Has this been merged in?

@jordanpotter
Copy link

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

@alindsay55661
Copy link

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

@shadowedged
Copy link

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

vojtajina added a commit to vojtajina/karma that referenced this pull request May 6, 2013
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
Copy link
Contributor

Guys, please check out the PR #533

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants