Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

Console errors and warning plugin #1916

Closed
wants to merge 15 commits into from
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ testapp/inbrowsertest/
website/bower_components/
website/build/
website/docgen/build/
.idea
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be in your personal git ignore file not the project one.


chromedriver.log
libpeerconnection.log
changes.sh
xmloutput*
npm-debug.log

*.swp
*.swp
102 changes: 102 additions & 0 deletions plugins/console/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
var q = require('q');
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 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.

Copy link
Member

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 use q 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.

Copy link
Contributor Author

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


var testOut = {failedCount: 0, specResults: []};
Copy link
Contributor

Choose a reason for hiding this comment

The 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 testOut.specResults.push( you want testOut.specResults.assertions.push(


/**
* This plugin scans the log after each test and can fail on error and warning messages
Copy link
Member

Choose a reason for hiding this comment

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

Clarify that it scans the 'browser log'

*
* exports.config = {
* plugins: [{
* path: 'node_modules/protractor/plugins/console',
* failOnWarning: {Boolean} (Default - false),
* failOnError: {Boolean} (Default - true)
* }]
* };
*/
var ConsolePlugin = function () {
this.failOnWarning = false;
Copy link
Member

Choose a reason for hiding this comment

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

Style nits: We use 2 spaces for indent inside function blocks.

this.failOnError = true;
Copy link
Member

Choose a reason for hiding this comment

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

Style nits: no space between function and ()

};

/**
* 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) {
console.error(warning.level.name + ': ' + warning.message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use "console.warn" here?

});

errors.map(function (error) {
console.error(error.level.name + ': ' + error.message);
});
};

/**
* Parses the log and decides whether to throw an error or not
*
* @param config
* The config from the protractor config file
* @returns {Deferred.promise}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it's q.Promise

*/
ConsolePlugin.parseLog = function (config) {
var self = this;
var deferred = q.defer();
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 should use protractor.promise.defer()

var failOnWarning = config.failOnWarning || this.failOnWarning;
var failOnError = config.failOnError || this.failOnError;
this.getBrowserLog().then(function (log) {
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 if you return here, you can avoid the explicit deferred. (You'll be returning the promise returned by then which should be the same thing.)


var warnings = log.filter(function (node) {
return (node.level || {}).name === 'WARNING';
});

var errors = log.filter(function (node) {
return (node.level || {}).name === 'SEVERE';
});

if (warnings.length > 0 || errors.length > 0) {
self.logMessages(warnings, errors);
}

testOut.failedCount += (warnings.length > 0 && failOnWarning) ? 1 : 0;
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add a message to testOut.specResults to show what the error is - if you do this, you might not need to separately log the messages as well. The plugin will automatically log information about failed specResults.

testOut.failedCount += (errors.length > 0 && failOnError) ? 1 : 0;

deferred.resolve();
});

return deferred.promise;
};

/**
* Tear-down function used by protractor
*
* @param config
*/
ConsolePlugin.prototype.teardown = function (config) {
var audits = [];

audits.push(ConsolePlugin.parseLog(config));

return q.all(audits).then(function (result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

protractor.promise.all is probably safer than q.all

return testOut;
});
};

var consolePlugin = new ConsolePlugin();

exports.teardown = consolePlugin.teardown.bind(consolePlugin);

exports.ConsolePlugin = ConsolePlugin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this export used anywhere?

13 changes: 13 additions & 0 deletions plugins/console/spec/consoleConfig.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
var env = require('../../../spec/environment.js');
Copy link
Contributor

Choose a reason for hiding this comment

The 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 scripts/test.js. Basically change

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');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I was looking for that.
One quick question for you, because I don't think my tests are very good. I cant think of a way to test that the tests fail without failing the test if you get me :). Any idea if this is possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible! If you look at the tests for the ngHint plugin, you'll see both failureConfig.js and successConfig.js. Then, in the scripts/test.js, you see that successConfig.js is pushed onto passingTests, but if you scroll down near the bottom you see

// 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 failureConfig.js, and expect it to fail with these errors"

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
}]
};
21 changes: 21 additions & 0 deletions plugins/console/spec/spec.js
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();
});
});
33 changes: 33 additions & 0 deletions testapp/console/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<!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');
}
}]);
</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>