-
Notifications
You must be signed in to change notification settings - Fork 2.3k
chore(exitCodes): adding exit code for browser connect errors #3133
Changes from 4 commits
c7d3f05
1b077ab
ef513f6
d340ab2
83b4d4a
fe98c6d
81fb263
4b54ec8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,79 @@ | ||
import {Logger} from './logger2'; | ||
|
||
const CONFIG_ERROR_CODE = 105; | ||
const BROWSER_ERROR_CODE = 135; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
export class ProtractorError extends Error { | ||
static ERR_MSGS: string[]; | ||
static DEFAULT_MSG = 'protractor encountered an error'; | ||
|
||
export class ProtractorError { | ||
error: Error; | ||
description: string; | ||
code: number; | ||
stack: string; | ||
constructor(logger: Logger, description: string, code: number) { | ||
super(); | ||
this.error = new Error(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have both inheritance and composition - if we're inheriting from error can we just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, not inherit? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a weird work around because |
||
this.description = description; | ||
this.code = code; | ||
this.description = description; | ||
this.stack = this.error.stack; | ||
this.logError(logger); | ||
} | ||
|
||
logError(logger: Logger) { | ||
logger.error('error code: ' + this.code); | ||
logger.error('description: ' + this.description); | ||
this.stack = this.error.stack; | ||
logger.error(this.stack); | ||
} | ||
} | ||
|
||
/** | ||
* Configuration file error | ||
*/ | ||
export class ConfigError extends ProtractorError { | ||
static DEFAULT_MSG = 'configuration error'; | ||
static CODE = CONFIG_ERROR_CODE; | ||
constructor(logger: Logger, description: string) { | ||
super(logger, description, ConfigError.CODE); | ||
constructor(logger: Logger, opt_msg?: string) { | ||
super(logger, opt_msg || ConfigError.DEFAULT_MSG, ConfigError.CODE); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why have a default configuration msg? 'configuration error' is pretty useless by itself, I'd say keep this required. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing the default message and plan to use the message from the thrown error. |
||
process.exit(ConfigError.CODE); | ||
} | ||
} | ||
|
||
/** | ||
* Browser errors including getting a driver session, direct connect, etc. | ||
*/ | ||
export class BrowserError extends ProtractorError { | ||
static DEFAULT_MSG = 'browser error'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto for not having a default error, make the error-thrower say something useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
static CODE = BROWSER_ERROR_CODE; | ||
static ERR_MSGS = [ | ||
'ECONNREFUSED connect ECONNREFUSED', 'Sauce Labs Authentication Error', | ||
'Invalid username or password' | ||
]; | ||
constructor(logger: Logger, opt_msg?: string) { | ||
super(logger, opt_msg || BrowserError.DEFAULT_MSG, BrowserError.CODE); | ||
process.exit(BrowserError.CODE); | ||
} | ||
} | ||
|
||
export class ErrorHandler { | ||
static isError(errMsgs: string[], e: Error): boolean { | ||
if (errMsgs && errMsgs.length > 0) { | ||
for (let errPos in errMsgs) { | ||
let errMsg = errMsgs[errPos]; | ||
if (e.message.indexOf(errMsg) !== -1) { | ||
return true; | ||
} | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
static parseError(e: Error): number { | ||
if (ErrorHandler.isError(ConfigError.ERR_MSGS, e)) { | ||
return ConfigError.CODE; | ||
} | ||
if (ErrorHandler.isError(BrowserError.ERR_MSGS, e)) { | ||
return BrowserError.CODE; | ||
} | ||
return null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
*/ | ||
import * as q from 'q'; | ||
import {Config, ConfigParser} from './configParser'; | ||
import {ConfigError, ErrorHandler} from './exitCodes'; | ||
import {Logger} from './logger2'; | ||
import {Runner} from './runner'; | ||
import {TaskRunner} from './taskRunner'; | ||
|
@@ -176,7 +177,18 @@ let initFn = function(configFile: string, additionalConfig: Config) { | |
// 4) Run tests. | ||
let scheduler = new TaskScheduler(config); | ||
|
||
process.on('uncaughtException', (e: Error) => { | ||
let errorCode = ErrorHandler.parseError(e); | ||
if (errorCode) { | ||
logger.error(e.stack); | ||
process.exit(errorCode); | ||
} else { | ||
logger.error(e.stack); | ||
} | ||
}); | ||
|
||
process.on('exit', (code: number) => { | ||
console.log(code); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be logging this? If someone wants to use it, they can just use the exit code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And it's noted below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
if (code) { | ||
logger.error('Process exited with error code ' + code); | ||
} else if (scheduler.numTasksOutstanding() > 0) { | ||
|
@@ -212,9 +224,8 @@ let initFn = function(configFile: string, additionalConfig: Config) { | |
1) { // Start new processes only if there are >1 tasks. | ||
forkProcess = true; | ||
if (config.debug) { | ||
throw new Error( | ||
'Cannot run in debug mode with ' + | ||
'multiCapabilities, count > 1, or sharding'); | ||
throw new ConfigError(logger, | ||
'Cannot run in debug mode with multiCapabilities, count > 1, or sharding'); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
#!/usr/bin/env node | ||
|
||
'use strict'; | ||
|
||
var spawn = require('child_process').spawnSync; | ||
var exitCodes = require('../built/exitCodes'); | ||
|
||
var runProtractor, output, messages; | ||
var checkLogs = function(output, messages) { | ||
for (var pos in messages) { | ||
var message = messages[pos]; | ||
if (output.indexOf(message) === -1) { | ||
throw new Error('does not exist in logs: ' + message); | ||
} | ||
} | ||
}; | ||
|
||
/****************************** | ||
*Below are exit failure tests* | ||
******************************/ | ||
|
||
// assert authentication error for sauce labs | ||
runProtractor = spawn('node', | ||
['bin/protractor', 'spec/errorTest/sauceLabsAuthentication.js']); | ||
output = runProtractor.stdout.toString(); | ||
messages = ['WebDriverError: Sauce Labs Authentication Error.', | ||
'Process exited with error code ' + exitCodes.BrowserError.CODE]; | ||
checkLogs(output, messages); | ||
|
||
// assert authentication error for browser stack | ||
runProtractor = spawn('node', | ||
['bin/protractor', 'spec/errorTest/browserStackAuthentication.js']); | ||
output = runProtractor.stdout.toString(); | ||
messages = ['WebDriverError: Invalid username or password', | ||
'Process exited with error code ' + exitCodes.BrowserError.CODE]; | ||
checkLogs(output, messages); | ||
|
||
|
||
// assert there is no capabilities in the config | ||
runProtractor = spawn('node', | ||
['bin/protractor', 'spec/errorTest/debugMultiCapabilities.js']); | ||
output = runProtractor.stdout.toString(); | ||
messages = [ | ||
'Cannot run in debug mode with multiCapabilities, count > 1, or sharding', | ||
'Process exited with error code ' + exitCodes.ConfigError.CODE]; | ||
checkLogs(output, messages); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
var env = require('../environment.js'); | ||
|
||
exports.config = { | ||
browserstackUser: 'foobar', | ||
browserstackKey: 'foobar', | ||
|
||
framework: 'jasmine', | ||
|
||
specs: [ | ||
'../../example/example_spec.js' | ||
], | ||
|
||
capabilities: { | ||
'browserName': 'chrome' | ||
}, | ||
|
||
baseUrl: env.baseUrl + '/ng1/', | ||
|
||
jasmineNodeOpts: { | ||
defaultTimeoutInterval: 90000 | ||
} | ||
|
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
var env = require('../environment.js'); | ||
|
||
exports.config = { | ||
seleniumAddress: env.seleniumAddress, | ||
framework: 'jasmine', | ||
debug: true, | ||
specs: [ | ||
'../../example/example_spec.js' | ||
], | ||
multiCapabilities: [{ | ||
'browserName': 'chrome' | ||
},{ | ||
'browserName': 'firefox' | ||
}], | ||
baseUrl: env.baseUrl, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
var env = require('../environment.js'); | ||
|
||
exports.config = { | ||
sauceUser: 'foobar', | ||
sauceKey: 'foobar', | ||
|
||
framework: 'jasmine', | ||
|
||
specs: [ | ||
'../../example/example_spec.js' | ||
], | ||
|
||
capabilities: { | ||
'browserName': 'chrome' | ||
}, | ||
|
||
baseUrl: env.baseUrl + '/ng1/', | ||
|
||
jasmineNodeOpts: { | ||
defaultTimeoutInterval: 90000 | ||
} | ||
|
||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this 'request' removal be split into its own PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.