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

Browser testing #451

Merged

Conversation

subtleGradient
Copy link
Contributor

No description provided.


module.exports = function() {
var config = this.data;
var done = this.async();

var theFilesToTestScript = fs.createWriteStream(__dirname + '/../../test/the files to test.generated.js');
Copy link
Contributor

Choose a reason for hiding this comment

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

How confident are we about spaces in file names? I'd be tempted to just use underscores here.

@subtleGradient
Copy link
Contributor Author

It's not passing every browser in testling-ci yet. I'm investigating.
Cf. https://ci.testling.com/subtleGradient/react

@@ -6,7 +6,7 @@ var global = {};
importScripts("phantomjs-shims.js");

try {
importScripts("react.js");
importScripts("../../build/react.js");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this requires us to run grunt build before grunt test, so that build/react.js will be built. Wondering if we should make grunt test depend on grunt build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not 100% sure about this change. I did it so that I could get the tests running in a browser normally without having to rely on the phantomjs web server contortions.

@benjamn
Copy link
Contributor

benjamn commented Oct 29, 2013

Can we not expect Testling-CI to run grunt for us?

var done = false;
var error;

var worker = new Worker('/worker.js');
var worker = new Worker(window.ReactWebWorker_URL || '/src/test/worker.js?_=' + Date.now().toString(36));
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @spicyj who last worked with ReactWebWorker-test: thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No strong feelings, I was expecting someone to want to change the generic path earlier than now. Can we not just use a relative path here though? I think I'm actually just missing context on the purpose of this PR.

@subtleGradient
Copy link
Contributor Author

I don't know how to get testling-ci to run grunt.
I do know that if you include a path in the "tests" section of the "testling" package.json thing it'll get run through browserify first. So maybe there's some way to piggyback on that somehow.

return a.pathname;
}());

var __dirname = __filename.split('/').reverse().slice(1).reverse().join('/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you've got your own scope here, why not just

var parts = __filename.split('/');
parts.pop();
var __dirname = parts.join('/');

@benjamn
Copy link
Contributor

benjamn commented Oct 29, 2013

@subtleGradient can you look into why Travis CI tests are failing?

Looks like something to do with ReactWebWorker timing out: https://travis-ci.org/facebook/react/builds/13216897

@subtleGradient
Copy link
Contributor Author

@benjamn looking into it now…

document.write('<script src="' + __dirname + '/../test/the-files-to-test.generated.js' + cacheBust + '"><\/script>');
document.write('<script src="' + __dirname + '/../test/jasmine-execute.js' + cacheBust + '"><\/script>');

}());
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I see the point in making this a dynamic JS file instead of generating a simple HTML file. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

testling-ci can load scripts like this, but it won't load an html file.

@subtleGradient
Copy link
Contributor Author

Got testling-ci working with IE10 and probably more.
React apparently fails a few tests.

@subtleGradient
Copy link
Contributor Author

Switching to SauceLabs…

@benjamn
Copy link
Contributor

benjamn commented Oct 29, 2013

👍 looks legit

@subtleGradient
Copy link
Contributor Author

The grunt-saucelabs implementation of jasmineRunner seems to be broken.
I'll have to fix it.

@@ -0,0 +1,37 @@
;(function(){
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of this file if we're using SauceLabs instead of Testling?

Copy link
Contributor

Choose a reason for hiding this comment

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

Still want to get rid of this file if possible.

@subtleGradient
Copy link
Contributor Author

I decided to remove testling and sauce labs as dependencies for this PR. I'll add sauce labs support in a separate PR.

grunt.loadNpmTasks('grunt-compare-size');
grunt.loadNpmTasks('grunt-contrib-compress');
for (var key in grunt.file.readJSON("package.json").devDependencies) {
if (key !== "grunt" && key.indexOf("grunt") === 0) grunt.loadNpmTasks(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just if (key.indexOf("grunt-") === 0) grunt.loadNpmTasks(key);?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just pasted this from somewhere. Dang my slackerly ways!

@benjamn
Copy link
Contributor

benjamn commented Nov 6, 2013

Looking great! My final request is to make grunt test --debug (same command as before) keep the server running so you can run tests in local browser.

@subtleGradient
Copy link
Contributor Author

You can run tests in a local browser by opening the test/index.html file, no server necessary.
If you need to keep the localhost:9999 server running for some reason you can use grunt connect:server:keepalive.

I'm not sure how to make it so that grunt test --debug runs something similar to grunt connect:server:keepalive, but I'm sure it's possible.

@subtleGradient
Copy link
Contributor Author

This is what I've been doing to test changes really quickly…

  1. Run chromedriver in one terminal window.[1]
  2. Turn on the localhost:9999 server using grunt connect:server:keepalive in another terminal window.
  3. Run grunt webdriver-jasmine:local in yet another terminal window.[2]

If you also want to test changes to any of the generated files you'll need to rebuild them as necessary with grunt build or whatever more specific task. This part could be made a little faster.

[1] Any webdriver server running on localhost:9515 will be used by the webdriver-jasmine:local task. You can use chromedriver or phantomjs --webdriver=9515 or whatever else you like. The webdriver-phantomjs task is essentially the same as phantomjs --webdriver=9515 except it kills it once its done.

[2] To keep the browser window open, pass the --webdriver-keep-open flag to grunt. e.g., grunt webdriver-jasmine:local --webdriver-keep-open. You can then refresh this page occasionally to manually test changes after rebuilding.

benjamn added a commit that referenced this pull request Nov 7, 2013
@benjamn benjamn merged commit b3c87ea into facebook:master Nov 7, 2013
@subtleGradient subtleGradient deleted the subtlegradient/browser-testing branch November 7, 2013 18:27
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