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

Update karma to run both firefox & chrome tests #894

Merged
merged 1 commit into from
Nov 21, 2016

Conversation

benjaminapetersen
Copy link
Contributor

Updating Karma to run both tests in both Firefox & Chrome concurrently:

  • Firefox is much more valuable since we all tend to use Chrome
  • Chrome, however, has much better error messages that make you less likely to throw yourself out the window while writing tests.

screen shot 2016-11-18 at 12 54 38 pm

@jwforres @spadgett

@jwforres
Copy link
Member

This has to be optional. From my testing a few months ago Chrome does not work well inside a container, which means its probably not going to work well in Travis either. Our tests have to be runnable inside a container for when we move to testing all of our stuff on openshift itself

@jwforres
Copy link
Member

With most of us developing in Chrome anyway, I'm not sure we get much benefit from running the spec tests in Chrome.

@benjaminapetersen
Copy link
Contributor Author

  • Right, let me see if I can make it optional
  • Agree, I'd rather always run them in Firefox or any non-chrome. During dev though, Firefox's error reporting is inferior, switching to Chrome is much easier to debug (only reason I did this).

@benjaminapetersen benjaminapetersen force-pushed the karma-update branch 2 times, most recently from b45638b to 14fd5d0 Compare November 21, 2016 18:46
@benjaminapetersen
Copy link
Contributor Author

how about this? can now set the browser else default to FF:

browsers=Chrome grunt test

@jwforres
Copy link
Member

you should do that with a grunt option not an environment variable for consistency with how we do all of our options

@benjaminapetersen
Copy link
Contributor Author

tried that, grunt doesn't exist in the karma.conf.js file. Wasn't sure if I should import grunt into that file & use grunt.options('browser'). Thoughts?

@jwforres
Copy link
Member

looks like you can override anything in the config file in the Gruntfile https://github.com/karma-runner/grunt-karma#heres-an-example-that-puts-the-config-in-the-gruntfile

@benjaminapetersen
Copy link
Contributor Author

Aha, I tried this in karma.conf.js:

    karma: {
      unit: {
        configFile: 'test/karma.conf.js',
        singleRun: true,
        browsers: grunt.options('browsers') ? [grunt.options('browsers')] : ['Firefox']
      }
    },

But it doesn't work cuz no grunt. Started to import, then realized I should prob do this in gruntfile.js instead:

    karma: {
      unit: {
        configFile: 'test/karma.conf.js',
        singleRun: true,
        // defaulting to Firefox for running tests, but recommend Chrome for writing
        // tests due to better error messages:
        //      grunt test --browsers=Chrome
        browsers: grunt.option('browsers') ? [grunt.option('browsers')] : ['Firefox']
      }
    }

Seems better?

@jwforres
Copy link
Member

Yeah generally looks like the right direction to me.
But that doesn't look like it would work correctly if multiple browsers
were specified, if you are only supporting 1 browser running then make the
flag browser (not plural)

On Mon, Nov 21, 2016 at 2:52 PM, Ben Petersen [email protected]
wrote:

Aha, I tried this in karma.conf.js:

karma: {
  unit: {
    configFile: 'test/karma.conf.js',
    singleRun: true,
    browsers: grunt.options('browsers') ? [grunt.options('browsers')] : ['Firefox']
  }
},

But it doesn't work cuz no grunt. Started to import, then realized I
should prob do this in gruntfile.js instead:

karma: {
  unit: {
    configFile: 'test/karma.conf.js',
    singleRun: true,
    // defaulting to Firefox for running tests, but recommend Chrome for writing
    // tests due to better error messages:
    //      grunt test --browsers=Chrome
    browsers: grunt.option('browsers') ? [grunt.option('browsers')] : ['Firefox']
  }
}

Seems better?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#894 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABZk7eQiZD8Gyy5jll8DSC_goYaCgASlks5rAfZzgaJpZM4K2wUp
.

@benjaminapetersen
Copy link
Contributor Author

😄
yeah, have been trying to figure out multiple, not quite there yet.

@jwforres
Copy link
Member

i'd probably just go with comma separated and do a string split if the
option exists

On Mon, Nov 21, 2016 at 3:03 PM, Ben Petersen [email protected]
wrote:

😄
yeah, have been trying to figure out multiple, not quite there yet.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#894 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABZk7d6RLcxGlQli3AJ8zT006ShbkVcVks5rAfkigaJpZM4K2wUp
.

@benjaminapetersen
Copy link
Contributor Author

Nice, that works, no quiry 'multiple options for grunt.options()' needed.

grunt test
grunt test --browsers=Chrome
grunt test --browsers=Chrome,Firefox

@jwforres
Copy link
Member

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to c6f3267

@openshift-bot
Copy link

openshift-bot commented Nov 21, 2016

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/750/) (Base Commit: 23682f1)

@openshift-bot openshift-bot merged commit c5349e7 into openshift:master Nov 21, 2016
@benjaminapetersen benjaminapetersen deleted the karma-update branch November 22, 2016 14:20
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.

3 participants