Use karma runner for test execution #236

Closed
wants to merge 27 commits into
from

Projects

None yet

7 participants

@markelog
Member

This PR would be used as basis for jQuery Core changes of the same nature.

  • These changes still let you run test through QUnit testsuite and now through karma too
  • If user has defined browserstack variables, browserstack browsers will be used
  • Pull request are not run through browserstack, since travis reasonably do not allow env vars for forks, phantomjs and "recent version of Firefox" (which is currently 19) will be used instead

/cc @Krinkle

@timmywil timmywil and 1 other commented on an outdated diff Dec 31, 2013
dist/sizzle.js
@@ -6,7 +6,7 @@
* Released under the MIT license
* http://jquery.org/license
*
- * Date: 2013-12-20
+ * Date: 2013-12-24
@timmywil
timmywil Dec 31, 2013 Member

Let's not update the Sizzle distribution unless there's a change.

@markelog
markelog Dec 31, 2013 Member

my bad, fixed

@timmywil timmywil and 1 other commented on an outdated diff Dec 31, 2013
test/unit/selector.js
- expect(numAssertions)
- stop()
- start()
- note: QUnit's eventual addition of an argument to stop/start is ignored in this test suite
- so that start and stop can be passed as callbacks without worrying about
- their parameters
- Test assertions:
- ok(value, [message])
- equal(actual, expected, [message])
- notEqual(actual, expected, [message])
- deepEqual(actual, expected, [message])
- notDeepEqual(actual, expected, [message])
- strictEqual(actual, expected, [message])
- notStrictEqual(actual, expected, [message])
- raises(block, [expected], [message])
-
======== testinit.js reference ========
See data/testinit.js
@timmywil
timmywil Dec 31, 2013 Member

Let's remove these lines if we're going to remove the QUnit reference.

@markelog
markelog Dec 31, 2013 Member

testinit reference? I found it quite useful.

@timmywil
timmywil Dec 31, 2013 Member

Then don't remove the QUnit reference. It was also added because some found it useful. Removing it is out of the scope of this PR anyway.

@markelog
markelog Jan 3, 2014 Member
It was also added because some found it useful

I too found it useful, but problem with this kind of comments is that they become stale pretty quickly, for example raises or notStrictEqual are not in the QUnit anymore, but this comment suggest otherwise, http://docs.jquery.com/QUnit link also is incorrect and so forth.

QUnit has great public documentation whereas data/testinit.js reference is part only of this repo, if you, for example, want to change/add some tests/assertions to selector module for the first time, you would have to face methods like t, without knowing origin of that function it could be pretty difficult to grasp its purpose.

Removing it is out of the scope of this PR anyway.

You right, rebased, but i still would like to remove it.

@timmywil
Member

👍 Nice work

@markelog
Member
markelog commented Jan 3, 2014

@timmywil now it's looks finished, but i didn't check integration with jQuery Core testsuite, given amount of browsers on which sizzle will be tested you still want that?

I know we talked about it before, but amount of hacks and undocumented features we use in order for it to work is horrible.

@timmywil
Member
timmywil commented Jan 3, 2014

@markelog No, I agree it's not worth it to continue integration with Core. Thank you for doing all of this.

@markelog
Member

If no one objects, this PR should be landed after removal of integration tests for jQuery Core which should happen right after final release of 2.1.0/1.11.0

@jzaefferer jzaefferer commented on the diff Jan 13, 2014
.travis.yml
@@ -1,3 +1,11 @@
language: node_js
node_js:
- '0.10'
+env:
+ global:
+ - secure: "j17tHs7MhF1VuJ7ChImA0a3kd+uHPri2HJJxJ++PKCEF7pAu/cksMq9v3xQp9ZajPJJ0z4gTQnMjrzUSjhL11cpweBy84suA/vqjoh4Rp6ouV9yv/wThcij6GHd0S9MzYol6b3iR1CCauQpQwy+dff1HJvcGrcheuxD62EhVzoI="
@jzaefferer
jzaefferer Jan 13, 2014 Member

As far as I understand this setup, there variables won't be available when Travis runs tests for PRs. So in that case, npm test would run the PR with PhantomJS?

@markelog
markelog Jan 13, 2014 Member

@jzaefferer and Firefox, see third item in PR description

@jzaefferer jzaefferer commented on the diff Jan 13, 2014
README.md
@@ -45,12 +45,12 @@ Testing
----------------------------
- Run `npm install`, it's also preferable (but not necessarily) to globally install `grunt-cli` package – `npm install -g grunt-cli`
-- Open `test/index.html` in the browser or run `npm test` or `grunt test` on the command line
+- Open `test/index.html` in the browser. Or run `npm test`/`grunt test` on the command line, if environment variables `BROWSER_STACK_USERNAME` and `BROWSER_STACK_ACCESS_KEY` are set up, it will attempt to use [Browserstack](http://www.browserstack.com/) service, otherwise [PhantomJS](http://phantomjs.org/) will be used.
@jzaefferer
jzaefferer Jan 13, 2014 Member

A link describing how to properly set up those variables and where to get the access key would be useful here.

@markelog
markelog Jan 13, 2014 Member

Good point will be updated.

@jzaefferer
jzaefferer Apr 9, 2014 Member

Looks like this was never updated, otherwise GitHub would hide these as outdated, right?

@markelog
markelog Apr 9, 2014 Member

Thought about, and can't come up with good explanation of how set up env vars and where to get access key, if user doesn't know about these things, or about browserstack service, he shouldn't do them

@jzaefferer jzaefferer and 1 other commented on an outdated diff Jan 13, 2014
test/karma/environment.js
+ // Support: IE6
+ // Have to re-arrange socket.io transports by prioritizing "jsonp-polling"
+ // otherwise IE6 can't connect to karma server
+ config.transports = [ "websocket", "flashsocket", "jsonp-polling", "xhr-polling" ];
+
+ if ( isTravis ) {
+
+ // Browserstack launcher specifies "build" options as a default value
+ // of "TRAVIS_BUILD_NUMBER" variable, but this way a bit more verbose
+ config.browserStack.build = "Travis #" + process.env.TRAVIS_BUILD_NUMBER;
+ }
+
+ // You can't get access to secure environment variables from pull requests
+ // so we don't have browserstack from them, but travis has headless Firefox so use that
+ } else if ( isTravis && process.env.TRAVIS_PULL_REQUEST ) {
+ config.browsers.push( "Firefox" );
@jzaefferer
jzaefferer Jan 13, 2014 Member

Is this provided via SlimerJS?

@jzaefferer
Member

This looks promising overall. Assuming it works as intended (haven't tried yet), my main concern for wider usage across our projects would be the amount of configuration files we need. grunt-karma is nice, but we'd probably need something like grunt-karma-jquery to share most of that setup across our projects. Not a blocker for this PR, but something to consider for next steps.

That said, another candidate for using Karma would be QUnit itself, since it only relies on static HTML (no PHP dependency). Maybe land this PR mostly as-is, then look into abstracting, and use the abstraction here and for QUnit?

@markelog
Member
grunt-karma-jquery
Yeah, i thought about it too, but i fear there is not a lot of parts that we can share, at least right now, but i will definitely check it out.
on static HTML

Even if project has some moving parts it would be easy enough to solve, my main concern is that testswarm executes every module in parallel, if you, for example, try to run a lot of tests like for jQuery Core, say in IE8, it would fail, but if you grep and run failed individual tests or modules they will pass.

This too could be workarounded but it would be a nightmare, so right now, i'm not entirely sure if Karma would be a good fit for "big" projects.

@jzaefferer
Member

I'm confused about these:

testswarm executes every module in parallel

if Karma would be a good fit for "big" projects.

Did you mean to refer to Karma in the first one as well?

Either way, what do you consider a "big" project? We don't that many projects with frontend test. Looking at http://jenkins.jquery.com/ we have jQuery Color, jQuery Core, jQuery Migrate, jQuery Mobile, jQuery UI and QUnit. Where do you see Karma as a good or bad fit for these?

@markelog
Member
Did you mean to refer to Karma in the first one as well?

No, swarm executes test modules in parallel not in a sense that it run some amount browsers simultaneously, but run them through specific modules like ?module=traversing, ?module=manipulation, etc; not whole testsuite at once not all tests at once, i believe swarm does it for speed, but as a result, jQuery Core testsuite is now falling.

In order to do the same with karma, we have to get clever if we would have to do this.

what do you consider a "big" project?

By "big" project i mean the one that support a lot of old browsers and have a lot of tests, so

if you, for example, try to run a lot of tests like for jQuery Core, say in IE8, it would fail, but if you grep and run failed individual tests or modules they will pass.

In other words, if testsuite could execute all tests without falling in all supported browsers, then it would be a easy enough to add Karma, if not, then it probably be a pretty hard task to manage.

@mgol
Member
mgol commented Jan 26, 2014

Many projects run tests in real browsers in Travis, e.g. via Sauce Labs. Are you sure it's not possible to do it with BrowserStack? IMO it would be very valuable to run tests in all supported browsers.

If not, maybe we could use Sauce Labs here anyway? It's free for OSS projects.

@markelog
Member

Don't understand the question, could please be more explicit? With these changes, tests are run in all supported browsers, including the mobile ones.

It's free for OSS projects.
same with Browserstack.
@mgol
Member
mgol commented Jan 29, 2014

@markelog

Don't understand the question, could please be more explicit? With these changes, tests are run in all supported browsers, including the mobile ones.

I mean for PRs, you wrote that it's impossible because of setting environmental variables. Many GitHub projects are running tests on SauceLabs and they're running on all major desktop browsers. Is that impossible to do with BrowserStack?

@markelog
Member
environmental variables

Not "environmental variables" but "secure environmental variables". This is neither saucelabs nor browserstack nor a travis restriction. We can run tests in all browsers for forks, but in order to do that, we would have to publisize our credentials, i hope i don't need to explain why it's bad move.

If you know projects that do that, they do a very stupid thing and you should create an issue for them.

@markelog
Member
you wrote that it's impossible
Can you point me to that quote?
@markelog
Member

I see, you looked at

since travis reasonably do not allow env vars for forks
Yes, in that sentence word "secure" is omitted, although it was implied given PR context.

@mgol
Member
mgol commented Jan 29, 2014

@markelog

you wrote that it's impossible
Can you point me to that quote?

I was only referring to your 3rd point in the PR description

environmental variables

Not "environmental variables" but "secure environmental variables". This is neither saucelabs nor browserstack nor a travis restriction. We can run tests in all browsers for forks, but in order to do that, we would have to publisize our credentials, i hope i don't need to explain why it's bad move.

If you know projects that do that, they do a very stupid thing and you should create an issue for them.


No, it's not insecure as you can encrypt the access key, see: https://saucelabs.com/opensource/travis
An example is Lo-Dash: https://github.com/lodash/lodash/blob/master/.travis.yml

@markelog
Member

@mzgol see my previous comment

@mgol
Member
mgol commented Jan 29, 2014

@markelog OK, I see.

BTW, I found one repository that has these keys unencrypted. ;) https://github.com/angular/angular.js/blob/master/.travis.yml

@markelog
Member

Yeah, i know, they could publish gmail private key while they at it. I had to twice notify browserstack support about it, until they finally created a ticket, but browserstack support seems don't understand how asymmetric cryptography works, because this –

I can't submit a pull request for this change
is incorrect, but when i did tell them about it, they didn't understand what i was saying.
@mgol
Member
mgol commented Jan 29, 2014

Well, they could provide a PR for current key but that makes no sense, the key should be changed and the new one should not be published.

But it's getting OT so let's move it there.

@markelog
Member

I wasn't talked about how angular issue could be resolved, but about these kind of misconceptions that started this discussion

@vojtajina

Hi guys,

you can run tests on multiple browsers in parallel with Karma. We do that on Angular - a single Travis VM runs Karma which captures multiple browsers on SauceLabs.

You can do the same with BrowserStack, although there are still some issues when running multiple jobs in parallel (multiple SSH tunnels). I'm working on this with BS guys so that we can use both SL and BS.

Regards Karma and big projects - it's definitely a goal for Karma. There are pretty big projects at Google using Karma. It's not possible to parallelize a single test suite right now but it's a planned feature (Karma is designed to make it possible and efficient).

As for QUnit - the integration is not as good as with Jasmine right now, because we use Jasmine. However, I'm happy to help you to improve the integration with QUnit.

Last, the problem of encrypting env variables on Travis (for pull requests). At this point there is no solution for this. I wanna get together with people from SauceLabs, BrowserStack and Travis and see if we can figure out something. Is there anybody from the jQ team that could be involved in this discussion too? Shoot me email please. @jzaefferer ?

@jzaefferer
Member

For the record, @vojtajina has started the thread he described, so far we've got responses from SauceLabs. My current impression: Once there's a solution for Travis/SauceLabs, implementing the same for BrowserStack will be a lot easier as well. And once Travis can runs tests on SL/BS for PRs, we have a huge incentive to make use of that, over our current Jenkins/TestSwarm setup.

@markelog
Member
markelog commented Feb 5, 2014

@jzaefferer could you include me as cc?

If SauceLabs is okay with it, we could temporary publish our keys to SL, it's not a problem to use it instead of Browserstack

@mgol mgol commented on an outdated diff Feb 5, 2014
test/karma/environment.js
+ // if Browserstack is set up, assume we can use it
+ if ( process.env.BROWSER_STACK_USERNAME && process.env.BROWSER_STACK_ACCESS_KEY ) {
+
+ // See http://jquery.com/browser-support/
+ config.browsers.push(
+ "chrome-30", "chrome-31",
+
+ "firefox-24", "firefox-25",
+
+ "ie-6", "ie-7", "ie-8", "ie-9", "ie-10", "ie-11",
+
+ "opera-12.16", "opera-16", "opera-17",
+
+ "safari-5.1", "safari-6.1", "safari-7",
+
+ "ios-5", "android-2.3", "android-4.1"
@mgol
mgol Feb 5, 2014 Member
  1. Android executes tests extremely slow, do we want it set up in the same place as the rest of the browsers? To give some data, jQuery Core test suite finishes in about 30 minutes on Android.

EDIT: OK, Sizzle tests run for less than a minute on Android so we're OK here (contrary to Core).

  1. You should add iOS6 here, Core is currently tested against that and Sizzle should always be tested against a superset of browsers compared to Core.
@mgol mgol and 1 other commented on an outdated diff Feb 5, 2014
test/karma/environment.js
+ var isTravis = process.env.TRAVIS;
+
+ // if Browserstack is set up, assume we can use it
+ if ( process.env.BROWSER_STACK_USERNAME && process.env.BROWSER_STACK_ACCESS_KEY ) {
+
+ // See http://jquery.com/browser-support/
+ config.browsers.push(
+ "chrome-30", "chrome-31",
+
+ "firefox-24", "firefox-25",
+
+ "ie-6", "ie-7", "ie-8", "ie-9", "ie-10", "ie-11",
+
+ "opera-12.16", "opera-16", "opera-17",
+
+ "safari-5.1", "safari-6.1", "safari-7",
@mgol
mgol Feb 5, 2014 Member

Safari 6.1 is identical to Safari 7 engine-wise so there's no need to test it here. OTH, you're missing Safari 6.0 which is the real previous version to Safari 7.

@markelog
markelog Feb 6, 2014 Member

We could add safari 4 and 6, unfortunately not 3 since it's missing in BS

@markelog
markelog Feb 6, 2014 Member

Talk about what browser version test in sizzle could be a long one (since sizzle support pretty old browsers) and it's not heavily relates to this PR.

We can augment it after (if) it lands.

@markelog
markelog Feb 6, 2014 Member

Safari 6.1 is identical to Safari 7 engine-wise

It's not identical btw, webkit versions are different and there is a plenty of resources that confirm this (for example), how did you determined which one of them is "real"?

@mgol
mgol Feb 6, 2014 Member

I'd prefer to have it fixed now (it's only 1 digit after all) since it's easy to overlook it and think that Safari 6.0 is covered by 6.1 whereas this makes Safari 7 covered twice and 6.0 left out. Safari numbering scheme is confusing.

@markelog
markelog Feb 6, 2014 Member

It's not one digit, see launchers module and see my previous comment and as i said, what browsers we need to test is a long and debatable question, we shouldn't clubber this discussion, this PR mostly provides an answer for "how" not for "what".

@mgol
mgol Feb 6, 2014 Member
Safari 6.1 is identical to Safari 7 engine-wise

It's not identical btw, webkit versions are different and there is a plenty of resources that confirm this (for example), how did you determined which one of them is "real"?


I don't understand your link, it doesn't say anything about 6.1/7 differences. I also just confirmed on BrowserStack that both 6.1 and 7.0 have the same WebKit version, i.e. 537.71, what is your source?

Anyway, wherever I've read about standards-related changes, Safari 6.1 & 7.0 were described together.

It's not one digit, see `launchers` module and see my previous comment and as i said, what browsers we need to test is a long and debatable question, we shouldn't clubber this discussion, this PR mostly provides an answer for "how" not for "what".

OK, I'll leave it here then. I still think it'd be good to update it all just before merging and wanted to point out the mistake here but if you insist it can be done later.

@markelog
markelog Feb 6, 2014 Member
it doesn't say anything about 6.1/7 differences

This link shows the difference between the two, hence "What’s new in Safari and WebKit" phrase.

i.e. 537.71, what is your source?

For Safari 6.0 and Safari 7.0 but Safari 7.0.1 is 537.73 and we do not specify the third dot

OK, I'll leave it here then. I still think it'd be good to update it all just before merging and wanted to point out the mistake here but if you insist it can be done later.

First of all we are not leaving it, we will have more in depth discussion about what we should put there, adding another version of Safari is not sufficient and this is not a mistake.

@markelog
markelog Feb 6, 2014 Member

To hell with it :-), you obviously feel strongly about it, will update it before the merge

@mgol
mgol Feb 6, 2014 Member
it doesn't say anything about 6.1/7 differences

This link shows the difference between the two, hence "What’s new in Safari and WebKit" phrase.


Where does it show any difference? I don't see it.

i.e. 537.71, what is your source?

For Safari 6.0 and Safari 7.0 but Safari 7.0.1 is 537.73 and we do not specify the third dot


Safari 6.1 not 6.0. Also, 6.1.1 has 537.73, just like 7.0.1. They're kept in parallel.

@markelog
markelog Feb 6, 2014 Member

Where does it show any difference? I don't see it.

You see the picture and words on it (seriously?)? There is your difference, you can peruse the links in there too.

Safari 6.1 not 6.0

It's a typo

Also, 6.1.1 has 537.73, just like 7.0.

Again, we do not specify the third dot, they work on two different platforms can you give the guarantees they will be updated on BS in that balance? Even if they will be updated consistanly it would not promise us that those versions would not disperse, it does not promise us that JS engine would behave the same, or if they difference in interfaces would not affect rendering in some way, it's just not safe. It better to leave it then remove it, so we probably will add another two versions of Safari because it does not cost us anything.

Now you understand what i mean when i said that discussion should be in another thread? We are not talking about Karma and it's implementation in this project, but about something little and unimportant, it's unproductive and mostly useless.

Please consider this thread closed.

@mgol
mgol Feb 6, 2014 Member

Where does it show any difference? I don't see it.

You see the picture and words on it (seriously?)? There is your difference, you can peruse the links in there too.

You said they're differences between 6.1 & 7.0. They're not, they're differences between 6.0 & 6.1/7.0.

Please consider this thread closed.

OK, if you want it.

@mgol mgol and 1 other commented on an outdated diff Feb 5, 2014
test/karma/launchers.js
+ os: "OS X",
+ os_version: "Mountain Lion"
+ },
+ "safari-7": {
+ base: "BrowserStack",
+ browser: "safari",
+ browser_version: "7.0",
+ os: "OS X",
+ os_version: "Mavericks"
+ },
+
+ "ios-5": {
+ base: "BrowserStack",
+ device: "iPhone 5",
+ os: "ios",
+ os_version: "6.0"
@mgol
mgol Feb 5, 2014 Member

This doesn't match the ios-5 name. Also, please add iOS6.

@markelog
markelog Feb 6, 2014 Member

Good call, will do

@mgol
Member
mgol commented Feb 5, 2014

@markelog I added some comments, not everything is OK yet in the PR. Anyway, awesome work. :)

@jzaefferer
Member

We need a plan for abstracting the test/karma files, since the effort of maintaining those in multiple places would be awful. I'm okay with leaving them here until we get a second project adopting Karma, but we need a least a plan for how to approach that. Might be a plain node module, a grunt task or whatever else.

@markelog
Member
markelog commented Feb 6, 2014

@mzgol everything is fine with this PR, it's older then a month now so it's okay that browser version are stale now, i'm not gonna update this periodically, when (if) i'm gonna land it, i will update browser versions.

@jzaefferer what you think we can share here? Right now, i'm only see launchers module, but if project would use it and it would want to update some browsers it would have to do it in two places instead of one.

But it is all declaration stuff, no logic here, it's simple as it gets. So how you would want to do it?

@mgol
Member
mgol commented Feb 6, 2014

@markelog Not all my remarks were about outdated browsers, e.g. you made a mistake with including Safari 6.1 instead of 6.0.

@markelog
Member
markelog commented Feb 6, 2014

i know @mzgol that's why i respond to them independently

@mgol
Member
mgol commented Feb 6, 2014

@markelog @jzaefferer IMO we could create a module that would generate proper browser strings from data about current versions of all supported browsers (where do we take that from btw?) and browser support rules, e.g. IE9+ would be currently expanded to IE9, IE10, IE11 (don't pay attention to specific strings, I'm describing only logic here), whereas Firefox: current, current - 1 would evaluate to Firefox 25, Firefox 26.

@markelog
Member
markelog commented Feb 6, 2014

@mzgol great idea, BS has API that could get us all supported browsers so we can generate it from there, but some browsers do not exist on some platforms, so it might be tricky but doable.

@mgol
Member
mgol commented Feb 6, 2014

So far we've tested each browser version only on one of systems it's available on. A simple loop over available Windows/OS X versions should suffice, the 3rd BrowserStack API makes it quite easy. (I wanted to make sure but http://api.browserstack.com/3/browsers/ returns 502 currently).

EDIT: obviously, the logic for mobile devices would be slightly different but it shouldn't be too hard anyway.

@mgol mgol and 1 other commented on an outdated diff Feb 6, 2014
test/karma/environment.js
@@ -0,0 +1,48 @@
+"use strict";
+
+module.exports = function( config ) {
+ var isTravis = process.env.TRAVIS;
+
+ // if Browserstack is set up, assume we can use it
+ if ( process.env.BROWSER_STACK_USERNAME && process.env.BROWSER_STACK_ACCESS_KEY ) {
+
+ // See http://jquery.com/browser-support/
+ config.browsers.push(
@mgol
mgol Feb 6, 2014 Member

Actually, why not:

config.browsers.concat( Object.keys( require( "./launchers" ) ) );

?

@markelog
markelog Feb 6, 2014 Member

Finally dude :-), now we talking, awesome idea, it would nicely fit with dynamically generated launchers module

@mgol mgol and 1 other commented on an outdated diff Feb 6, 2014
test/karma/environment.js
@@ -0,0 +1,48 @@
+"use strict";
+
+module.exports = function( config ) {
+ var isTravis = process.env.TRAVIS;
+
+ // if Browserstack is set up, assume we can use it
+ if ( process.env.BROWSER_STACK_USERNAME && process.env.BROWSER_STACK_ACCESS_KEY ) {
+
+ // See http://jquery.com/browser-support/
@mgol
mgol Feb 6, 2014 Member

This shows browsers supported by jQuery, not Sizzle. Probably should be changed to https://github.com/jquery/sizzle/wiki/Sizzle-Documentation#wiki-browsers

@markelog
markelog Feb 6, 2014 Member

Good catch

@markelog markelog referenced this pull request in jquery/jquery Feb 18, 2014
Closed

Remove Sizzle integration test, fix #14818 #1517

@mgol
Member
mgol commented Mar 24, 2014

I was thinking about it and it seems we do need to have concrete browsers versions defined in the repo. We should generate the list using some wildcards, probably sth like I proposed in a Google doc but we should keep the exact generated list in the repo, similarly as we currently do with our Bower dependencies.

In this way if we ever need to ship a patch update for an older release when we already dropped support for some of browser versions in newer releases we can still test it on browsers we declared as supported in this older version (this can happen if we need to issue a patch 1.12.x release when we already shipped 1.13 where we drop IE6/7 amongst others).

See also jquery/infrastructure#249

cc @jzaefferer @dmethvin @Krinkle

@markelog
Member
markelog commented Apr 5, 2014

I made some final changes, testing in browserstack browsers is more flexible and stable, although
i had to temporary remove Opera 12.16 from the list, since it's unstable with karma right know, browserstack is already notified, so it should be fixed soon, i hope at least.

I would like to merge this, even if there is additional concern so we could move this along.

@mgol
Member
mgol commented Apr 5, 2014

@markelog

i had to temporary remove Opera 12.16 from the list, since it's unstable with karma right know

Did you try with Opera 12.15, 12.14 etc.? They have a lot of these patch versions around (which is not the case for most other browsers).

Do you want to add missing browsers in later PRs? Because currently the tested matrix is way too small compared to what needs to be tested.

@mgol mgol and 1 other commented on an outdated diff Apr 5, 2014
grunt: [ "Gruntfile.js", "tasks/*" ]
};
+ // if Browserstack is set up, assume we can use it
+ if ( process.env.BROWSER_STACK_USERNAME && process.env.BROWSER_STACK_ACCESS_KEY ) {
+
+ // See https://github.com/jquery/sizzle/wiki/Sizzle-Documentation#browsers
+
+ browsers.desktop = [
@mgol
mgol Apr 5, 2014 Member

This should be changed to

browser.desktop = Object.keys( require( "./test/karma/launchers.js" ) );

per our earlier discussion.

@markelog
markelog Apr 5, 2014 Member

I didn't forget, but did think about it a lot, but the bottom line is: it easier to filter browsers like that, then add/remove them from launchers module

@mgol
mgol Apr 5, 2014 Member

OK, we'll probably switch to getting rid of test/karma/launchers.js being written manually and we'll generate this file from our browser matrix so that we don't have to update it manually later.

@markelog
markelog Apr 5, 2014 Member

My thoughts exactly

@markelog
Member
markelog commented Apr 5, 2014

Did you try with Opera 12.15, 12.14 etc.?

Yes, different OS, versions, timeouts, locally it works fine, just like it works fine with browserstack live Opera, but through java connections it's dumping connections.

Do you want to add missing browsers in later PRs?

Most def, current list is not enough, but we could have additional troubles with this for old browsers, since socket.io might not work with them

@mgol
Member
mgol commented Apr 5, 2014

OK then. I added a comment but everything can be changed in subsequent commits if you wish so.

@markelog
Member
markelog commented Apr 5, 2014

OK then.

Will wait until opinions from our heavy hitters... or monday :-)

@mgol mgol commented on an outdated diff Apr 5, 2014
},
"scripts": {
"build": "npm install && grunt",
"start": "grunt watch",
+ "install": "grunt bower",
@mgol
mgol Apr 5, 2014 Member

What is this needed for?

@mgol mgol commented on an outdated diff Apr 5, 2014
test/unit/selector.js
-// #### NOTE: ####
+// // #### NOTE: ####
@mgol
mgol Apr 5, 2014 Member

why this change?

@markelog
Member
markelog commented Apr 5, 2014

@mzgol typos are fixed.

@mgol
Member
mgol commented Apr 7, 2014

@markelog Do you plan to squash all those commits?

@mgol mgol and 1 other commented on an outdated diff Apr 7, 2014
test/data/fixtures.html
+ <div id="t2037">
+ <div><div class="hidden">hidden</div></div>
+ </div>
+ <div id="t6652">
+ <div></div>
+ </div>
+ <div id="t12087">
+ <input type="hidden" id="el12087" data-comma="0,1"/>
+ </div>
+ <div id="no-clone-exception"><object><embed></embed></object></div>
+ <div id="names-group">
+ <span id="name-is-example" name="example"></span>
+ <span id="name-is-div" name="div"></span>
+ </div>
+ <script id="script-no-src"></script>
+ <script id="script-src" src=""></script>
@mgol
mgol Apr 7, 2014 Member

This will load the current page as a script at least in some browsers. I don't think that's what you wanted.

@markelog
markelog Apr 7, 2014 Member

Fixed that before, probably lost that commit in all the rebases i did

@markelog
markelog Apr 7, 2014 Member

Although fix for that, is pretty hackish, at least right now :(

@mgol mgol commented on the diff Apr 7, 2014
test/karma/karma.conf.js
+
+ // If browser does not capture in given timeout [ms], kill it
+ captureTimeout: 3e5,
+ browserNoActivityTimeout: 3e5
+ });
+
+ // Deal with Travis environment
+ if ( isTravis ) {
+
+ // Browserstack launcher specifies "build" options as a default value
+ // of "TRAVIS_BUILD_NUMBER" variable, but this way a bit more verbose
+ config.browserStack.build = "travis #" + process.env.TRAVIS_BUILD_NUMBER;
+
+ // You can't get access to secure environment variables from pull requests
+ // so we don't have browserstack from them, but travis has headless Firefox so use that
+ if ( !(process.env.BROWSER_STACK_USERNAME && process.env.BROWSER_STACK_ACCESS_KEY) &&
@mgol
mgol Apr 7, 2014 Member

I'd name it BROWSERSTACK_USERNAME and BROWSERSTACK_ACCESS_KEY since BrowserStack is a single word (similarly how I'd write GITHUB and not GIT_HUB as the name is one word - GitHub).

@markelog
markelog Apr 7, 2014 Member

Then you probably should create ticket here :), we can of of course workaround this, but i wouldn't.

@mgol
mgol Apr 7, 2014 Member

Ah, right; I thought it was your choice. Since it's in Karma & probably has been there for some time now, I agree it'd add more harm than value to change it now.

@mgol mgol and 1 other commented on an outdated diff Apr 7, 2014
test/karma/launchers.js
+ browser: "opera",
+ browser_version: "12.16",
+ os: "Windows",
+ os_version: "8.1"
+ },
+ "bs_opera-19": {
+ base: "BrowserStack",
+ browser: "opera",
+ browser_version: "19.0",
+ os: "OS X",
+ os_version: "Mavericks"
+ },
+ "bs_opera-20": {
+ base: "BrowserStack",
+ browser: "opera",
+ browser_version: "17.0",
@mgol
mgol Apr 7, 2014 Member

There should be 20.0 here.

@markelog
markelog Apr 7, 2014 Member

Thanks again!

@mgol
Member
mgol commented Apr 7, 2014

I've just run it locally, works fine. :) You made a typo in Opera version (see inline comment).

It's really cool to finally be able to test on all browsers without pushing to master. :)

@mgol
Member
mgol commented Apr 7, 2014

@dmethvin @timmywil @gibson042 @jzaefferer Any further comments/LGTMs? It'd be great to merge it soon; this is awesome. :)

@jzaefferer
Member

It's really cool to finally be able to test on all browsers without pushing to master. :)

Yes, it is indeed. I'm concerned how much effort it is to set this up though. I still think we need to get support of loading tests as HTML files into Karma before we can seriously consider adopting it. Last I talked to Oleg about this, he was confident that its something that can be done and their team would at least be interested.

Since the conference in San Diego I've worked with the BrowserStack team on improving their browserstack-runner project. Its not quite where I want it to be, though I've been using it for jQuery UI for a while now. The configuration is dead-simple and trivial to maintain (here's a browserstack.json for this repo). Running it against Sizzle's test suite I ran into two issues, which I expect to get fixed in the next days. Its not perfect, but it allows us to test on all browsers without pushing to master with a fraction of the effort, compared to configuring Karma.

Since the gist don't capture the actual output, this is a screenshot of an incomplete run:

browserstack-runner screenshot

markelog added some commits Dec 31, 2013
@markelog @markelog markelog Tests: Remove debug artefacts 3de64a7
@markelog @markelog markelog Tests: Remove redundant instructions c984576
@markelog @markelog markelog Build: Move karma config to test folder
* Add "empty.js" file into the karma server
* Add and correct some comments in karma config
* Small simplification of instructions of the karma grunt task
* Remove karma dependency from the package.json since it's a global package
4ab1ecf
@markelog @markelog markelog Build: Use browserstack through karma runner 9a603bc
@markelog @markelog markelog Tests: Deal with empty.js path
With "/base/test/data/empty.js" path karma runner will not show 404 errors,
for local run through actual browser, 404 path will be okay.

Hackinsh, but it's probably the best thing that we can do.
20097f1
@markelog @markelog markelog Build: Change karma initialization strategy
* Add secure environment variables

* Instead of the "browsers" module use "launchers" to store
  browserstack browser launchers info and "environment" which
  would change karma settings for different environments
a1ca942
@markelog @markelog markelog Build: Specify browserstack settings f0d9118
@markelog @markelog markelog Build: Clean up
* Lint karma modules
* Prettify some statements
* Use "use strict" statement in karma modules
f9fdc58
@markelog @markelog markelog Docs: Reflect karma related changes in the readme 5be5fc2
@markelog @markelog markelog Build: Execute tests in Firefox from forks 685a7d7
@markelog @markelog markelog Build: Reset karma browsers option at watch task
Even if developer has browserstack access, do not use it to run all
supported browsers at watch task, since it's execution will take a long time
4c813bb
@markelog markelog Build: Add more useful "test" task
With it you can specify browsers you want to run tests.
Example: grunt test --browsers=chrome-30,android_2.3
72e4d99
@markelog markelog Build: Improve flexibility of karma setup
* Remove environment module by moving it's logic to grunt and karma config

* Declare new "test" task in grunt config

* If browserstack settings are not available use only phantomjs instead

* Add prefix "bs_" for browserstack browsers
8cdf20f
@markelog markelog Build: Divide browsers on groups
To make testing process more flexible,
divide browsers on three groups: desktop, old and mobile

Update browsers version but don't use Opera 12.16, since it's behaves unpredictable
1dce98c
@markelog markelog Build: Add missed karma packages fed2c8d
@markelog markelog Tests: Improve karma config
* Give a name for browserstack build of local runs

* Remove transports option from karma config

* Use lowercase for name of travis build of browserstack

* Increase timeouts so runs through browserstack would be more stable
5295bc3
@markelog
Member
markelog commented Apr 7, 2014

he was confident that its something that can be done and their team would at least be interested

I hope i can do that, it looks like karma config could became more simple, so it would be like browserstack runner is, but more over, it would be nice to have some addtional features in QUnit, like qunitjs/qunit#496 and qunitjs/qunit#350, so work through Karma would be more comfortable, i.e. one thing is to make it run tests on every commit like testswarm, another, is to locally test things, that is something that browserstack runner is lack of, at least was, last time i checked.

@jzaefferer
Member

I hope i can do that, it looks like karma config could became more simple

That would be a good first step. There's still a lot going on here.

but more over, it would be nice to have some addtional features in QUnit

Extending QUnit.begin needs no discussion. I've commented on the other ticket.

to locally test things, that is something that browserstack runner is lack of, at least was, last time i checked.

Correct, browserstack-runner doesn't do that and very likely never will. I've never missed this ability, unlikely the feature that it does provide. I'm not sure how important that is.

@gibson042
Member

You're right; it is cool to watch the tests run on all browsers. And I think it's satisfactory enough to land now and make tweaks later.

Unless I'm mistaken, though, this is still blocked by jquery/jquery#1517 and jquery/jquery#1555.

@markelog
Member

Unless I'm mistaken, though, this is still blocked by jquery/jquery#1517 and jquery/jquery#1555.

As long as i can see, they are ready, i was planning to merge this and PRs above simultaneously

@dmethvin
Member

I tried getting this to run on Windows yesterday, but PhantomJS won't start. Seems to be common and I attempted several of the things mentioned here but no luck. The discussion here is outdated but as far as I can tell everything looks good in the Gruntfile. I will look into it a bit more later today.

@markelog markelog Build: Update "karma-phantomjs-launcher" package
In older version there was a problem with Windows laucnher of phantomjs
059cfc2
@markelog
Member

@dmethvin fixed

markelog added some commits Apr 10, 2014
@dmethvin
Member

Awesome, getting closer! The tests start at BrowserStack but it seems to hang at that point.
capture

@mgol
Member
mgol commented Apr 10, 2014

Dave, it's going to take a while. :D If you have the same output after 30
minutes, we have a problem (try to re-run it, though).

@markelog
Member

I'm seeing the problem too, working on that

@markelog
Member

P.S. hate windows

@gibson042
Member

Unless I'm mistaken, though, this is still blocked by jquery/jquery#1517 and jquery/jquery#1555.

As long as i can see, they are ready, i was planning to merge this and PRs above simultaneously

👍

@markelog
Member

@dmethvin Since browserstack requires java bin to establish protected tunnel, you need to install java, i will mention this in the readme.

@mgol
Member
mgol commented Apr 10, 2014

Maybe there should be a simple check for Java before running the tests?

Michał Gołębiowski

@markelog
Member

@mzgol probably, but not on our side

@dmethvin
Member

Java, 😿 Yep, that got it going! It would be good to have some way to detect that it's missing if it's a global dependency.

Got an "executed 0 of 0" on the IE8/XP test but that was called a success. Should that be an error?

@markelog
Member

Got an "executed 0 of 0" on the IE8/XP test but that was called a success. Should that be an error?

At the beginning - yes, after that it should have been changed, if you got to the message

Done, without errors.

Then everything should be fine, you might have missed it, there is a lot of logs flying around, but just to be safe, try to do:

grunt karma:old

or

grunt test --browsers=bs_ie-8

or both, btw i have fix ready for qunitjs/qunit#350, so this kind of stuff would evident in the future.

@markelog
Member

it's missing if it's a global dependency

Should be pretty easy to do, i need to not forget about it, will try send PR for it to the associated project.

@dmethvin
Member

I think this looks awesome! Seems good to go.

@jzaefferer
Member

Btw. the issues I mentioned earlier that browserstack-runner had are now resolved. I reran with 0.1.12 and it finished without failures. It logged two errors though, based on the CLI output I can't tell where those came from.

If anyone is interested, this gist has the output and the configuration I used: https://gist.github.com/jzaefferer/080dee40df272e8a7595

Integrating this into Travis would require modifications to .travis.yml and package.json.

@markelog
Member

merged

@markelog markelog closed this Apr 15, 2014
@markelog markelog referenced this pull request Apr 15, 2014
Closed

Update browser list #258

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment