-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Console errors and warning plugin #1916
Changes from 9 commits
b65159f
0fd503d
3e9d669
1fb5b46
200eae8
8436f83
f953412
41590bb
2ff7192
e72bd90
65ba682
af97467
1bf7ab9
a4d4d10
8ea8466
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 |
---|---|---|
|
@@ -11,4 +11,4 @@ changes.sh | |
xmloutput* | ||
npm-debug.log | ||
|
||
*.swp | ||
*.swp |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
var q = require('q'); | ||
|
||
var testOut = {failedCount: 0, specResults: []}; | ||
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. You haven't quite conformed to the spec here. I think what you want is: var testOut = {failedCount: 0, specResults = [{
description: "Console output",
assertions: [],
duration: 0
}]}; And then, in later lines, when you write |
||
|
||
/** | ||
* This plugin checks the browser log after each test for warnings and errors. It can be configured to fail a test if either is detected. | ||
* There is also an optional exclude parameter which accepts both regex and strings, | ||
* Any log matching the exclude parameter will not fail the test or be logged to the console | ||
* | ||
* exports.config = { | ||
* plugins: [{ | ||
* path: 'node_modules/protractor/plugins/console', | ||
* failOnWarning: {Boolean} (Default - false), | ||
* failOnError: {Boolean} (Default - true) | ||
* exclude: {Array of strings and regex} (Default - []) | ||
* }] | ||
* }; | ||
*/ | ||
var ConsolePlugin = function() { | ||
}; | ||
|
||
/** | ||
* Gets the browser log | ||
* | ||
* @returns {!webdriver.promise.Promise.<!Array.<!webdriver.logging.Entry>>} | ||
*/ | ||
ConsolePlugin.getBrowserLog = function() { | ||
return browser.manage().logs().get('browser'); | ||
}; | ||
|
||
/** | ||
* Logs messages to the test output | ||
* | ||
* @param warnings | ||
* The list of warnings detected by the browser log | ||
* @param errors | ||
* The list of errors detected by the browser log | ||
*/ | ||
ConsolePlugin.logMessages = function(warnings, errors) { | ||
warnings.map(function(warning) { | ||
testOut.specResults.push({ | ||
description: warning.level.name, | ||
passed: false, | ||
assertions: [{passed: false, errorMsg: warning.message}] | ||
}); | ||
}); | ||
|
||
errors.map(function(error) { | ||
testOut.specResults.push({ | ||
description: error.level.name, | ||
passed: false, | ||
assertions: [{passed: false, errorMsg: error.message}] | ||
}); | ||
}); | ||
}; | ||
|
||
/** | ||
* Determines if a log message is filtered out or not. This can be set at the config stage using the exclude parameter. | ||
* The parameter accepts both strings and regex | ||
* | ||
* @param logMessage | ||
* Current log message | ||
* @returns {boolean} | ||
*/ | ||
ConsolePlugin.includeLog = function(logMessage) { | ||
return this.exclude.filter(function(e) { | ||
return (e instanceof RegExp) ? logMessage.match(e) : logMessage.indexOf(e) > -1; | ||
}).length === 0; | ||
}; | ||
|
||
/** | ||
* Parses the log and decides whether to throw an error or not | ||
* | ||
* @param config | ||
* The config from the protractor config file | ||
* @returns {Deferred.promise} | ||
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. nit: it's |
||
*/ | ||
ConsolePlugin.parseLog = function(config) { | ||
var self = this; | ||
var deferred = q.defer(); | ||
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. You don't need this return this.getBrowserLog().then(function(log) {
...
}); 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. Good idea will give it a shot |
||
var failOnWarning = config.failOnWarning || false; | ||
var failOnError = config.failOnError || true; | ||
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. BUG: even if someone explicitly sets 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. Hi Sammy I dont think that is a bug. I am seeing that the tests will pass if I set failOnError to false which is correct. Correct me if i am wrong but || is short hand for undefined null or empty, so the value will be set to true only if config.failOnError is undefined. And if it is initially set to false then it is defined and the value correctly will be set to false? 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 don't know how your tests as passing, but 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. You are correct. I will update this. Good catch |
||
this.exclude = config.exclude || []; | ||
|
||
this.getBrowserLog().then(function(log) { | ||
|
||
var warnings = log.filter(function(node) { | ||
return (node.level || {}).name === 'WARNING' && self.includeLog(node.message); | ||
}); | ||
|
||
var errors = log.filter(function(node) { | ||
return (node.level || {}).name === 'SEVERE' && self.includeLog(node.message); | ||
}); | ||
|
||
testOut.failedCount += (warnings.length > 0 && failOnWarning) ? 1 : 0; | ||
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. Right now if there are 5 warning that only increases |
||
testOut.failedCount += (errors.length > 0 && failOnError) ? 1 : 0; | ||
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. Same comment as above |
||
|
||
if(testOut.failedCount > 0) { | ||
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 you want to output the log regardless don't you? |
||
self.logMessages(warnings, errors); | ||
} | ||
|
||
deferred.resolve(); | ||
}); | ||
|
||
return deferred.promise; | ||
}; | ||
|
||
/** | ||
* Tear-down function used by protractor | ||
* | ||
* @param config | ||
*/ | ||
ConsolePlugin.prototype.teardown = function(config) { | ||
var audits = []; | ||
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 this function could be simplified down do: return ConsolePlugin.parseLog(config).then(function() {
return testOut;
}) 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. Hi Sammy I had initially set it up like this but ran into a strange issue where the test would finish before the parseLog function finished which meant testOut.failedCount was always zero and it would never fail the test. The only way I could get it to work in order was to wrap it in a promise 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 like the fact that it's in a promise! I'm just not clear why you put the promise in an array 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. Your correct. A mistake on my part as I originally had 2 promises being pushed onto the array. I will update this |
||
|
||
audits.push(ConsolePlugin.parseLog(config)); | ||
|
||
return q.all(audits).then(function(result) { | ||
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. style: |
||
return testOut; | ||
}); | ||
}; | ||
|
||
var consolePlugin = new ConsolePlugin(); | ||
|
||
exports.teardown = consolePlugin.teardown.bind(consolePlugin); | ||
|
||
exports.ConsolePlugin = ConsolePlugin; | ||
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. Is this export used anywhere? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
var env = require('../../../spec/environment.js'); | ||
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. In order to get this test to run on travis, you'll need to make an addition to passingTests.push(
'node lib/cli.js plugins/timeline/spec/conf.js',
'node lib/cli.js plugins/ngHint/spec/successConfig.js',
'node lib/cli.js plugins/accessibility/spec/successConfig.js'); to passingTests.push(
'node lib/cli.js plugins/timeline/spec/conf.js',
'node lib/cli.js plugins/ngHint/spec/successConfig.js',
'node lib/cli.js plugins/accessibility/spec/successConfig.js',
'node lib/cli.js plugins/console/spec/consoleConfig.js'); 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. Thanks I was looking for that. 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. It is possible! If you look at the tests for the // Check ngHint plugin
executor.addCommandlineTest(
'node lib/cli.js plugins/ngHint/spec/failureConfig.js')
.expectExitCode(1)
.expectErrors([{
message: 'warning -- ngHint plugin cannot be run as ngHint code was ' +
'never included into the page'
}, {
message: 'warning -- ngHint is included on the page, but is not active ' +
'because there is no `ng-hint` attribute present'
}, {
message: 'warning -- Module "xApp" was created but never loaded.'
}]); This basically means "run 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. Thats awesome, thanks for the help. Will try getting it to work this evening |
||
|
||
exports.config = { | ||
seleniumAddress: env.seleniumAddress, | ||
framework: 'jasmine2', | ||
specs: ['spec.js'], | ||
baseUrl: env.baseUrl, | ||
plugins: [{ | ||
path: '../index.js', | ||
failOnWarning: true, | ||
failOnError: true, | ||
exclude: [ | ||
'string', | ||
/regex/ | ||
] | ||
}] | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
describe('console plugin', function () { | ||
|
||
var logMessageButton = element(by.id('log-message')); | ||
var warningMessageButton = element(by.id('simulate-warning')); | ||
var deleteMessageButton = element(by.id('simulate-error')); | ||
|
||
it('not fail on log and debug messages', function () { | ||
browser.get('console/index.html'); | ||
logMessageButton.click(); | ||
}); | ||
|
||
it('should fail on warning message', function() { | ||
browser.get('console/index.html'); | ||
warningMessageButton.click(); | ||
}); | ||
|
||
it('should fail on error message', function() { | ||
browser.get('console/index.html'); | ||
deleteMessageButton.click(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
<!DOCTYPE html> | ||
|
||
<html ng-app="consolePlugin" lang="en"> | ||
<head> | ||
<meta charset="utf-8"> | ||
<title>Angular.js Example</title> | ||
<script src="../../lib/angular/angular.min.js"></script> | ||
<script src="../../lib/angular/angular-aria.min.js"></script> | ||
<script type="text/javascript"> | ||
angular.module('consolePlugin', []) | ||
.controller('TestCtrl', ['$scope', function($scope) { | ||
|
||
$scope.logMessage = function() { | ||
console.log('This is a test log'); | ||
console.debug('This is a test debug'); | ||
}; | ||
|
||
$scope.simulateWarning = function() { | ||
console.warn('This is a test warning'); | ||
}; | ||
|
||
$scope.simulateError = function() { | ||
console.error('This is a test error'); | ||
console.error('This should be filtered out by string'); | ||
console.error('This should be filtered out by regex'); | ||
} | ||
}]); | ||
</script> | ||
</head> | ||
<body ng-controller="TestCtrl"> | ||
<button id="log-message" ng-click="logMessage()">Log Message</button> | ||
<button id="simulate-warning" ng-click="simulateWarning()">Simulate Warning</button> | ||
<button id="simulate-error" ng-click="simulateError()">Simulate Error</button> | ||
</body> | ||
</html> |
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.
I think you should use protractor.promise instead of q. The webdriver code has its own promise implementation and you probably want to use the same infrastructure.
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.
It's super confusing, but Protractor actually uses both
q
and the webdriver promises. I think it's OK to useq
in this situation, because it has no need to sync up with the webdriver promises - and it's actually probably safer, because it can't accidentally interfere with the webdriver control flow.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.
Will use q for now as per Julies comment