feat: add environment details to results #1353

straker · 2019-02-06T18:58:53Z

Adds tool options, axe-core version, branding name, and environment details to the result object.

Closes issue: #947

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

Follows the commit message policy, appropriate for next version
Has documentation updated, a DU ticket, or requires no documentation change
Includes new tests, or was unnecessary
Code is reviewed for security by: @WilcoFiers

…nto versionInfo

stephenmathieson

Please add these new properties to the TypeScript definition file.

issues resolved

stephenmathieson

Comments and questions left inline.

axe.d.ts

stephenmathieson · 2019-02-07T17:20:26Z

lib/core/reporters/na.js

@@ -9,7 +9,25 @@ axe.addReporter('na', function(results, options, callback) {
 	}

 	var out = helpers.processAggregate(results, options);
+	var orientation =


We should probably use the standards-compliant object first:

var orientation = screen.orientation || screen.msOrientation || screen.mozOrientation || {}

Just incase someone defined a non-standard property of the same name.

Not a big deal either way tho.

We could, I just copied the MDN code to grab it https://developer.mozilla.org/en-US/docs/Web/API/Screen/orientation

🤷‍♀️ OK, that's fine I suppose

stephenmathieson · 2019-02-07T17:21:09Z

lib/core/reporters/no-passes.js

@@ -9,8 +9,25 @@ axe.addReporter('no-passes', function(results, options, callback) {
 	options.resultTypes = ['violations'];

 	var out = helpers.processAggregate(results, options);
+	var orientation =


Is it worth moving this into a getScreenOrientation function since it's duplicated across files?

Again, not a big deal IMO, just a thought.

lib/core/reporters/v2.js

stephenmathieson · 2019-02-07T17:26:53Z

test/core/reporters/na.js

@@ -245,4 +245,25 @@ describe('reporters - na', function() {
 			done();
 		});
 	});
+	it('should add version information', function(done) {
+		axe.run(naOption, function(err, results) {
+			try {


This try/catch seems very awkward. Can't we just do:

axe.run(naOption, function (err, results) { if (err) { return done(err) } // Assertions go here done() })

The try/catch is for the assertions. A failed assertion throws an error but done is not called with it, so the unit test just hangs until timeout, then stops the test there without reporting what the error was.

Ohh, wow, OK. That's something we should change. I wonder if updating mocha would solve the problem for us.

Obviously don't worry about this now.

stephenmathieson · 2019-02-07T17:27:16Z

test/core/reporters/no-passes.js

@@ -209,4 +209,25 @@ describe('reporters - no-passes', function() {
 			done();
 		});
 	});
+	it('should add version information', function(done) {
+		axe.run(noPassOpt, function(err, results) {
+			try {


Same comment about the try/catch.

stephenmathieson · 2019-02-07T17:27:28Z

test/core/reporters/v1.js

@@ -283,4 +283,25 @@ describe('reporters - v1', function() {
 			done();
 		});
 	});
+	it('should add version information', function(done) {
+		axe.run(optionsV1, function(err, results) {
+			try {


Same comment about the try/catch.

stephenmathieson · 2019-02-07T17:28:08Z

test/core/reporters/v1.js

+			try {
+				assert.isNull(err);
+				assert.isObject(results.testEngine, 'tesetEngine');
+				assert.equal(results.testEngine.name, 'axe-core');


Why are we testing the same thing 3 different times? Is there a case where 1 of these tests could fail but the others wouldn't? If not, I think we only need to do this once.

These are different reporters. Just answered my own question. Disregard my question.

stephenmathieson · 2019-02-07T17:28:23Z

test/core/reporters/v2.js

@@ -227,4 +227,25 @@ describe('reporters - v2', function() {
 			done();
 		});
 	});
+	it('should add version information', function(done) {
+		axe.run(optionsV2, function(err, results) {
+			try {


Same duplicate test and try/catch concern.

Made more sense of this after speaking with Steven

WilcoFiers · 2019-02-08T09:58:53Z

lib/core/reporters/na.js

@@ -9,7 +9,25 @@ axe.addReporter('na', function(results, options, callback) {
 	}

 	var out = helpers.processAggregate(results, options);
+	var orientation =
+		screen.msOrientation || (screen.orientation || screen.mozOrientation || {});


I don't think we can assume screen exists as a global (or at all). I kinda doubt this is available on IE9. Please check that it exists before accessing its properties.

WilcoFiers · 2019-02-08T10:05:00Z

lib/core/reporters/na.js

+			windowHeight: window.innerHeight,
+			orientationAngle: orientation.angle,
+			orientationType: orientation.type
+		},


Instead of duplicating this code a bunch of times, it'd be better to create a helper for this in lib/core/reporters/helpers, so you could just do:

callback({ ...getEnvironmentData(), toolOptions: options, violations: out.violations, passes: out.passes, incomplete: out.incomplete, inapplicable: out.inapplicable })

You could probably have the URL & timestamp returned from it too. That's probably a better place for those two than the processAggregate helper.

@WilcoFiers please see #1353 (comment)

We've discussed this already, but neither @straker nor I know where to put things like this.

created helper function

straker added 4 commits February 6, 2019 11:32

feat: add version information to results

0bb479c

feat: add version information to results

7036ab1

Merge branch 'versionInfo' of https://github.com/dequelabs/axe-core i…

e358af7

…nto versionInfo

use tabs instead of spaces

a53b9de

straker requested a review from a team as a code owner February 6, 2019 18:58

stephenmathieson previously requested changes Feb 6, 2019

View reviewed changes

add definitions to typescript

3612af8

stephenmathieson previously requested changes Feb 7, 2019

View reviewed changes

WilcoFiers previously requested changes Feb 8, 2019

View reviewed changes

create helper function for environment data

d006b6e

add done to tests

b4a8d14

stephenmathieson approved these changes Feb 8, 2019

View reviewed changes

WilcoFiers approved these changes Feb 11, 2019

View reviewed changes

Merge branch 'develop' into versionInfo

49a3485

WilcoFiers merged commit e795f7d into develop Feb 11, 2019

WilcoFiers deleted the versionInfo branch February 11, 2019 11:48

jeeyyy mentioned this pull request Apr 5, 2019

Output axe-core 3.2 new metadata in axe-cli dequelabs/axe-cli#93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add environment details to results #1353

feat: add environment details to results #1353

straker commented Feb 6, 2019 •

edited by WilcoFiers

Loading

stephenmathieson left a comment

stephenmathieson left a comment

stephenmathieson Feb 7, 2019

straker Feb 7, 2019

stephenmathieson Feb 7, 2019

stephenmathieson Feb 7, 2019

stephenmathieson Feb 7, 2019

straker Feb 7, 2019

stephenmathieson Feb 7, 2019

stephenmathieson Feb 7, 2019

stephenmathieson Feb 7, 2019

stephenmathieson Feb 7, 2019

stephenmathieson Feb 7, 2019

stephenmathieson Feb 7, 2019

WilcoFiers Feb 8, 2019

WilcoFiers Feb 8, 2019

stephenmathieson Feb 8, 2019

feat: add environment details to results #1353

feat: add environment details to results #1353

Conversation

straker commented Feb 6, 2019 • edited by WilcoFiers Loading

Reviewer checks

stephenmathieson left a comment

Choose a reason for hiding this comment

stephenmathieson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straker commented Feb 6, 2019 •

edited by WilcoFiers

Loading