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

Add cross browser testing with Karma and SauceLabs #1301

Closed

Conversation

alexstrat
Copy link
Contributor

This pull request adds cross-browser testing facilities thanks to Karma with access to various virtualized platforms and browsers thanks to Open Sauce plugged to current Travis CI configuration.

  • makes nodeunit test suite browser compatible with bc1ff64, 26ae977 and the file test/browser.js
  • adds karma and karma-nodeunit adapter in dfaae74: make test run on (local-)chrome
  • adds saucelabs support in c43e0d6 and add a task to make tests run on various browsers
  • adds necessary configuration to make karma+sauce run on Travis – but needs that you finish the integration.

Test suite can be run on a local Chrome with grunt test:browser.

To run test suite on saucelabs you need to create an open-source account on https://saucelabs.com/opensource and export your username/accesskey in env variables:

export SAUCE_USERNAME='your-username'
export SAUCE_ACCESS_KEY='your-accesskey'

then, run grunt test:sauce-browser that will run tests on various browsers: firefox on linux, ie8, ie9, safari on OSX and chrome on winxp.

To finish the integration for Travis, you need to follow this guide: https://saucelabs.com/opensource/travis and add encrypted username/accesskey to .travis.yml.

Caveats:

@icambron
Copy link
Member

Oh wow, this is awesome. I'm going to go through this soon and add specific comments.

I was planning to spend the day porting the tests to Mocha, but now I think I'll hold off for a bit.

@alexstrat
Copy link
Contributor Author

Porting tests to Mocha is still possible – just have to change the karma adapter. It would be great since Mocha is designed to be run on both node and browser, and largely used and maintained.

@icambron I made you collaborator of my fork so that you can push on this banch to add your encrypted username and accesskey.

@alexstrat
Copy link
Contributor Author

For ie8, I think I got hurt by a very silly bug of this kind: http://tobyho.com/2013/03/13/window-prop-vs-global-var/

So the only viable solution I found is wrapping every test files in a ;(function(exports){ /*content*/ }(this));.

However,

  • that makes jshint yelling at me because the indentation is wrong and adding indentations change nothing to that
  • the test is still failing on ie8 with weird error: IE 8.0.0 (Windows 7) ERROR Some of your tests did a full page reload! after some test-cases succeed. I try to isolate the test that does that but without success for the moment.

To wrap tests:

require('glob').sync('./test/**/*.js').forEach(function(file) {
  var lines = require('fs').readFileSync(file).toString().split('\n');

  // add an indentation level
  for(var i=0; i<lines.length; i++) {
    if(lines[i]) {
      //console.log(lines[i]);
      lines[i] = '        '+ lines[i];
      //console.log(lines[i]);
    }
  }

  lines.unshift('(function (exports) {');

  if(!lines[lines.length-1]) {
    lines[lines.length-1] = '}(this));';
  } else {
    lines.push('}(this));');
  }
  lines.push('');

  console.log(file);
  require('fs').writeFileSync(file, lines.join('\n'));
});

@ichernev
Copy link
Contributor

Thanks a lot @alexstrat. I think proper automated cross-browser testing is the last piece missing for making moment robust and reduce the error reports. I hope I'll have a chance to take a look at this in more detail over the holidays.

@jdalton
Copy link

jdalton commented Dec 6, 2013

👍

@ichernev
Copy link
Contributor

ichernev commented Dec 6, 2013

This is incredible! Thank you @alexstrat. You posted excellent instructions, I was able to set up saucelabs, I did the necessary steps for travis (I guess we can test once this goes upstream).

Now about the changes you made to all tests: I'll see if we can do something saner here, probably make sure that nodeunit is exported global and then attach tests directly to it, so we wouldn't need a browser.js. We can also generate that but if we can avoid it -- better.

About tests not working on IE8 -- I can also give it a try, the interface seems pretty straight forward, just like remote karma... what a gem :)

@jdalton
Copy link

jdalton commented Dec 6, 2013

I recently got tests running in sauce too, thanx to @pimterry, and it's fantastic. I'm using the rest api with tunnel ids, which is pretty new (and helping iron out the issues there). You can even test various IE compat modes \o/

The sauce labs folks are quick to reply (email/irc/twitter) and very helpful.

@ichernev
Copy link
Contributor

ichernev commented Dec 6, 2013

Ok, so I managed to get all the tests working on IE8... I didn't do anything special. Just wrap all tests in

;(function () {
/*...*/
}.call(this));

So if we're going through all the trouble we may as well use UMD for loading moment, and assign the tests somewhere on nodeunit, so running is not an issue.

@alexstrat you may give it another look, if not I'll merge it in the next few days and apply the necessary changes on top to make it work.

@alexstrat
Copy link
Contributor Author

@ichernev where can i have a look to your changes? It does not look like you pushed them on my branch..

Rather than hard-coding the wrapping, I wanted to try a Karma preprocessor that would wrap the files on the fly.
In addition to that, it could attach test cases to a global variables NODEUNIT_TESTCASES that would be passed to nodeunit (nodeunit.run(NODEUNIT_TESTCASES)) – similar to what you wrote upper.

@ichernev
Copy link
Contributor

ichernev commented Dec 7, 2013

Tests will run in three environments -- node, browser and karma. I prefer to engineer something (like use UMD for tests too) that would work for all of them rather than hack around it for karma only.

I can not commit to your branch, I can make a PR. Let me first examine the UMD hypothesis.

@alexstrat
Copy link
Contributor Author

@ichernev I made you collaborator of my fork few weeks ago, you should then be able to commit on this branch.

Karma is not a "environnement": karma is a runner/launcher/reporter framework of the "browser environment".

Can you explain your UMD solution further: I don't understand it.

@ichernev
Copy link
Contributor

ichernev commented Dec 7, 2013

UMD is universal module definition -- something that works with amd, commonjs and browser globals. But now that I think more about it converting the tests to mocha would be a better idea.

The way we currently run the tests in a browser (for momentjs.com) is we concatenate all test files, on the top we redefine require and exports, and at the bottom we use all defined tests (in exports) to launch nodeunit with them. There is also code to display broken tests and the like. I'm not sure if we want all that for the karma -- probably not.

If we use mocha instead, all test will have the beautiful define / it construction, they'd just work in node, for the browser you include mocha lib at the top and you hook to relevant events (test case failed/passed) to display results. For karma you just use karma-mocha plugin. No hacks, clean and simple.

@alexstrat
Copy link
Contributor Author

Oh for sure, moving to Mocha is the ideal solution. I considered doing it when I started working on that PR, but the amount of work discouraged me and it did not look necessary.. For info, I considered writing a conversion script based on falafel: might be interesting to explore.

I see what's UMD, but that's your solution I do not understand.

The karma preprocessor based solution would require to revert bc1ff64. That means test code won't change much compared to current master: your script that concatenates tests and redefine export/require should then work without changes. I think that it would be an acceptable (and pretty clean) "hack" while waiting to switch to Mocha.

@ichernev
Copy link
Contributor

ichernev commented Dec 7, 2013

For now we can put a concat grunt task that would produce min/all-tests.js, that would do the wrap magic, and then browser.html will just use the exported global, as you/I suggested. You can make this change.

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.

None yet

4 participants