Skip to content

Windows fixes #101

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 12, 2015
Merged

Windows fixes #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b92ffaf
Changed to BrowserStackLocal.exe instead of the jar-file, the tunnel …
Dec 9, 2014
28bca6c
Styling fixes and removed unnecessary regexp flag
Dec 11, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions bin/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ function launchServer() {
}

function launchBrowser(browser, path) {
var url = 'http://localhost:' + serverPort.toString() + '/' + path;
var url = 'http://localhost:' + serverPort.toString() + '/' + path.replace(/\\/g, '/');
var browserString = utils.browserString(browser);
logger.debug('[%s] Launching', getTestBrowserInfo(browserString, path));

Expand Down Expand Up @@ -224,7 +224,7 @@ var statusPoller = {
config.status = 1;
}

process.kill(process.pid, 'SIGTERM');
process.exit('SIGTERM');
}
}
}, activityTimeout * 1000);
Expand All @@ -245,7 +245,7 @@ var statusPoller = {
config.status = 1;
}

process.kill(process.pid, 'SIGTERM');
process.exit('SIGTERM');
}
}
}, (activityTimeout * 1000));
Expand Down Expand Up @@ -308,11 +308,8 @@ try {
runTests();
var pid_file = process.cwd() + '/browserstack-run.pid';
fs.writeFileSync(pid_file, process.pid, 'utf-8');
process.on('SIGINT', function() {
cleanUpAndExit('SIGINT', 1);
});
process.on('SIGTERM', function() {
cleanUpAndExit('SIGTERM', config.status);
process.on('exit', function(signal){
cleanUpAndExit(signal, config.status);
});
} catch (e) {
console.log(e);
Expand Down
4 changes: 4 additions & 0 deletions lib/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ if (commit_id) {
});

var formatPath = function(path) {
if (/^win/.test(process.platform)) {
path = path.replace(/\//g, '\\');
}

if (path.indexOf(pwd) === 0) {
path = path.slice(pwd.length + 1);
}
Expand Down
62 changes: 34 additions & 28 deletions lib/local.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,40 +1,32 @@
var Log = require('./logger'),
logger = new Log(global.logLevel),
exec = require('child_process').exec,
exec = require('child_process').execFile,
fs = require('fs'),
http = require('http'),
windows = ((process.platform.match(/win32/) || process.platform.match(/win64/)) !== null),
localBinary = process.cwd() + (windows ? '/BrowserStackTunnel.jar' : '/BrowserStackLocal'),
localBinary = process.cwd() + '/BrowserStackLocal' + (windows ? '.exe' : ''),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't test this myself, so: Since you're now using execFile, do you even need to specify the extension? Windows binaries on the command prompt can be executed without specifying the extension. I guess its necessary, but would be nice to simplify this code if it isn't.

Copy link
Author

Choose a reason for hiding this comment

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

I tried without the extension, it worked but was not recognized as an application in the explorer so I kept the extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that running BrowserStackLocal on the Windows command prompt would execute BrowserStackLocal.exe. I don't know if that same "shortcut" works in node, but should be worth a try.

Copy link
Author

Choose a reason for hiding this comment

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

Yes it works in node, but then the file is not recognized as an application in the explorer view. That is why I kept the use with extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still feel like we're not talking about the same thing. Consider this (fake console example):

$ dir
BrowserStackLocal.exe
$ BrowserStackLocal
Executes BrowserStackLocal.exe

Note that the second command doesn't specify the extension, but finds the file anyway.

Now if the file is named BrowserStackLocal.exe, what happens in node when you run execFile("BrowserStackLocal")?

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, then lets drop the ternary and just execute BrowserStackLocal.

Copy link
Author

Choose a reason for hiding this comment

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

You still do a check if the file exists on line 91. Where you use the same path as for execFile. I guess you want to save the BrowserStackLocal with extension? Then you have to add that there instead, or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

True, the exists check needs the full file name.

It seems fine to me to just save the file without the extension, shouldn't matter how its displayed in Explorer. I'll leave that to someone from BrowserStack to decide.

Copy link
Author

Choose a reason for hiding this comment

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

Let me know if I should change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

On a related note, fs.exists() isn't recommended for use, since you can't distinguish between a file not existing and an error during the check for existence. Instead, you should stat or just try to use the file. Alternatively, you could use the which module.

utils = require('./utils'),
config = require('./config');

var Tunnel = function Tunnel(key, port, uniqueIdentifier, callback) {
var that = {};

function tunnelLauncher() {
var tunnelCommand = (windows ? 'java -jar ' : '') + localBinary + ' ';
if (config.debug) {
tunnelCommand += ' -v ';
}
tunnelCommand += key + ' ';
tunnelCommand += 'localhost' + ',';
tunnelCommand += port.toString() + ',';
tunnelCommand += '0';
tunnelCommand += (typeof uniqueIdentifier === 'undefined') ? ' -force -onlyAutomate' : ' -tunnelIdentifier ' + uniqueIdentifier;
tunnelCommand += checkAndAddProxy();
var tunnelOptions = getTunnelOptions(key, uniqueIdentifier);

if (typeof callback !== 'function') {
callback = function(){};
}

logger.debug('[%s] Launching tunnel', new Date());
var subProcess = exec(tunnelCommand, function(error, stdout, stderr) {

var subProcess = exec(localBinary, tunnelOptions, function(error, stdout, stderr) {
logger.debug(stderr);
logger.debug(error);
if (stdout.indexOf('Error') >= 0 || error) {
logger.debug('[%s] Tunnel launching failed', new Date());
logger.debug(stdout);
process.kill(process.pid, 'SIGINT');
process.exit('SIGINT');
}
});

Expand Down Expand Up @@ -67,30 +59,44 @@ var Tunnel = function Tunnel(key, port, uniqueIdentifier, callback) {
that.process = subProcess;
}

function checkAndAddProxy() {
var proxy = config.proxy;
if (typeof proxy === 'undefined') {
return '';
function getTunnelOptions(key, uniqueIdentifier) {
var options = [key];

if (config.debug) {
options.push('-v');
}

if (!uniqueIdentifier) {
options.push('-force');
options.push('-onlyAutomate');
} else {
options.push('-localIdentifier ' + uniqueIdentifier);
}
var proxyCommand = '';
proxyCommand += ' -proxyHost ' + proxy.host;
proxyCommand += ' -proxyPort ' + proxy.port;
if(typeof proxy.username !== 'undefined'){
proxyCommand += ' -proxyUser ' + proxy.username;
proxyCommand += ' -proxyPass ' + proxy.password;

var proxy = config.proxy;

if (proxy) {
options.push('-proxyHost ' + proxy.host);
options.push('-proxyPort ' + proxy.port);

if (proxy.username && proxy.password) {
options.push('-proxyUser ' + proxy.username);
options.push('-proxyPass ' + proxy.password);
}
}
return proxyCommand;

return options;
}

fs.exists(localBinary, function(exists) {
if (exists) {
tunnelLauncher();
return;
}
logger.debug('Downloading BrowserStack Local to `%s`', localBinary);
logger.debug('Downloading BrowserStack Local to "%s"', localBinary);

var file = fs.createWriteStream(localBinary);
http.get(windows ? 'http://www.browserstack.com/BrowserStackTunnel.jar' : ('http://s3.amazonaws.com/browserStack/browserstack-local/BrowserStackLocal-' + process.platform + '-' + process.arch),
http.get((windows ? 'http://s3.amazonaws.com/browserStack/browserstack-local/BrowserStackLocal.exe' : ('http://s3.amazonaws.com/browserStack/browserstack-local/BrowserStackLocal-' + process.platform + '-' + process.arch)),
function(response) {
response.pipe(file);

Expand All @@ -101,7 +107,7 @@ var Tunnel = function Tunnel(key, port, uniqueIdentifier, callback) {
}, 100);
}).on('error', function(e) {
logger.info('Got error while downloading binary: ' + e.message);
process.kill(process.pid, 'SIGINT');
process.exit('SIGINT');
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion lib/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ exports.Server = function Server(bsClient, workers) {
config.status = 1;
}

process.kill(process.pid, 'SIGTERM');
process.exit('SIGTERM');
}
});
});
Expand Down
2 changes: 1 addition & 1 deletion lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ var alertBrowserStack = function alertBrowserStack(subject, content, params, fn)
if (typeof params === 'function') {
} else {
fn = function() {
process.kill(process.pid, 'SIGINT');
process.exit('SIGINT');
};
}
}
Expand Down