Update more gatherers to use gather error instead of -1 artifacts #1623
Conversation
| @@ -32,7 +32,7 @@ class AxeAudit extends Audit { | |||
| * @return {!AuditResult} | |||
| */ | |||
| static audit(artifacts) { | |||
| const violations = artifacts.Accessibility.violations || []; | |||
| const violations = artifacts.Accessibility.violations; | |||
There was a problem hiding this comment.
now asserted as an array in the gatherer since axe says any valid result will include the violations array.
| return Accessibility._errorAccessibility('Unable to parse axe results'); | ||
| } | ||
|
|
||
| if (returnedValue.error) { |
There was a problem hiding this comment.
This appears to be from working around issues in the old evaluateAsync before @patrickhulce fixed up the error handling to properly reject. So, removed.
|
|
||
| const Gatherer = require('./gatherer'); | ||
|
|
||
| class HTML extends Gatherer { |
There was a problem hiding this comment.
super old and unused
| @@ -44,18 +44,12 @@ class HTMLWithoutJavaScript extends Gatherer { | |||
| return options.driver.evaluateAsync(`(${getBodyText.toString()}())`) | |||
| .then(result => { | |||
| if (typeof result !== 'string') { | |||
| throw new Error('result was not a string'); | |||
| throw new Error('document body innerText returned by protocol was not a string'); | |||
There was a problem hiding this comment.
nit: document.body.innerText
| @@ -21,19 +21,6 @@ const assert = require('assert'); | |||
| /* eslint-env mocha */ | |||
|
|
|||
| describe('Offline: Service Worker audit', () => { | |||
| it('reports driver error when given no Service Worker versions', () => { | |||
There was a problem hiding this comment.
so all these tests are going through a single codepath now? https://github.com/GoogleChrome/lighthouse/pull/1591/files#diff-9476be2df7d812bc50da0dd73d73f2f8R205?
There was a problem hiding this comment.
More or less, if I understand what you're asking. Audits never receive an error artifact; runner produces an error audit result for them if any required artifacts are in error. That's tested by the test you linked.
There was a problem hiding this comment.
Yea ok. Just checking we're not losing test coverage.
| } | ||
| .evaluateAsync(expression) | ||
| .then(returnedValue => { | ||
| if (!returnedValue || !Array.isArray(returnedValue.violations)) { |
There was a problem hiding this comment.
since we're nuking all the remaining checks on the violations array, lets just add an assertion in accessibility-test.js that we throw in this case.
originally used as an old html minify example gather/audit - b08937c
brendankenny
force-pushed
the
nomanifest
branch
from
072a3cf
to
0aef351
Compare
followup to #1560, continuing work on #941. This removes catches, etc from gatherers, letting any exceptions handle themselves as gatherer errors. Separated by commit for easier review, but most of these are fairly straightforward.
This leaves only the Manifest gatherer and the deobetterweb gatherers to go; thought I'd break here before it got too unwieldy.