[webkit-reviews] review denied: [Bug 184071] AX: Audit Tab should have an Audit Manager : [Attachment 342727] Patch

Mon Jun 25 12:01:30 PDT 2018

Brian Burg <bburg at apple.com> has denied Aaron Chu <aaron_chu at apple.com>'s
request for review:
Bug 184071: AX: Audit Tab should have an Audit Manager
https://bugs.webkit.org/show_bug.cgi?id=184071

Attachment 342727: Patch

https://bugs.webkit.org/attachment.cgi?id=342727&action=review

--- Comment #10 from Brian Burg <bburg at apple.com> ---
Comment on attachment 342727
  --> https://bugs.webkit.org/attachment.cgi?id=342727
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342727&action=review

It's almost ready, mostly just style comments at this point. I want this to run
through EWS again after the test changes are made.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:46
> +    get reports() { 

This can simply be:

return [...this._reports.values()]

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:63
> +	   if (representedObject instanceof WI.AuditTestSuite) {

Nit: else if

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:65
> +	       this.dispatchEventToListeners(WI.AuditManager.Event.TestStarted,
{test: representedObject});

Are you sure you want to dispatch that event for the suite and the test case
starting? Each case will also fire these events. I'd not fire it here unless
the UI cared specifically about a suite starting/ending.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:68
> +	       let result = testCases.slice(1).reduce((chain, testCase, index)
=> {

I think you mean testCases.slice().reduce(...

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:79
> +		       return this._getAuditResultForTest(testCase)

Nit: ending ;

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:97
> +	       this.dispatchEventToListeners(WI.AuditManager.Event.TestEnded,
{test: representedObject});

Ditto, I think this is the wrong event to send.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:100
> +	   this._reports.set(representedObject.id, auditReport);

We'll probably need to send an event here (or at the beginning of the method)
about the new report being available so it can be shown in the UI.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:109
> +	   this._testSuiteConstructors.push(auditTestSuiteConstructor);

Why do we keep instances and constructors both?

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:113
> +    idForReport(reportId)

This seems misnamed, shouldn't it be reportForId?

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:125
> +    async _getAuditResultForTest(testCase)

I would rename this as '_runTestCase'.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:127
> +	   let failed = true;

Naming nit: use didRaiseException = false, and assign = true in the catch
block.

> Source/WebInspectorUI/UserInterface/Models/AuditReport.js:32
> +	   this._reportName = representedTest.name;

This variable doesn't seem necessary.

> Source/WebInspectorUI/UserInterface/Models/AuditReport.js:40
> +    get representedTest() { return this._representedTest; }

I'd prefer separate getters for representedTestCases (always returns an array,
possibly with one element) and representedTestSuite (can return null). This
will make it more obvious in the UI code that both cases need to be handled.

> Source/WebInspectorUI/UserInterface/Models/AuditReport.js:42
> +    get resultData() { return this._results.slice(); }

Nit: 'results'

> Source/WebInspectorUI/UserInterface/Models/AuditReport.js:43
> +    get warningCount() { return this._warningsResults.length; }

This code looks out of date w.r.t. the constructor, I think I said they could
just be removed right? I think it would be sufficient to have no such getters,
or expose failedTests and passedTests for more simplistic uses. The UI will
always need to fetch the full list of results before rendering anything.

> Source/WebInspectorUI/UserInterface/Models/AuditReport.js:53
> +    get severity()

I don't understand what this is supposed to be used for, can it be removed? I'd
prefer the UI have its own logic to determine what severity to display.

> Source/WebInspectorUI/UserInterface/Models/AuditResult.js:33
> +	   this._errorDetails = testInstance.errorDetails;

Huh?

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:43
> +	   this._id = Symbol(name);

Nit: newline

> Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:42
> +    get testCases() {

return [...this._testCases.values()]

> Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:51
> +    get testCaseCount()

What is the point of this? A test suite's cases won't change once it's been
run, so clients can just use .length on a previously accessed testCases result,
right?

> Source/WebInspectorUI/UserInterface/Models/AuditTestSuite.js:58
> +    _buildTestCasesFromDescriptors(descriptors)

Is it expected that all test suites will specify cases as descriptors in static
methods? If so, then this method should be called at the end of the
constructor. If not, then remove WI.NotImplementedError.subclassMustOverride
and make this code get run when setting testCaseDescriptors.

> LayoutTests/inspector/audit/audit-manager.html:12
> +	name: "Adding an AuditTestSuite",

Nit: weird indent, please use 4 spaces per indent

> LayoutTests/inspector/audit/audit-manager.html:57
> +	       })

Nit: missing ;

> LayoutTests/inspector/audit/audit-manager.html:67
> +			for (let i = 0; i < auditReport.resultData.length; i++)
{

Nit: weird indent

> LayoutTests/inspector/audit/audit-manager.html:109
> +	       await auditManager.runAuditTestByRepresentedObject(testSuite);

Another way to state the design/test is that only the most recent AuditReport
for a case/suite is retained.

> LayoutTests/inspector/audit/audit-manager.html:114
> +	       InspectorTest.expectThat(reports[0].name === testSuite.name,
"The report represents the correct AuditTestSuite.");

As written, this test would not catch a bug in the above design point. I would
have expected you to save the the generated reports like:

let results = [ await ..., await ...]

Then you can directly verify the above design.

InspectorTest.expectThat(reports[0] == results[1], ...)

> LayoutTests/inspector/audit/audit-manager.html:127
> +		let auditReportForTestSuite =
auditManager.idForReport(testSuite.id);

Ditto here, should catch the result of await (a report) and assert it's the
most recent run.

> LayoutTests/inspector/audit/audit-manager.html:133
> +		await auditManager.runAuditTestByRepresentedObject(testCase);

Ditto.

> LayoutTests/inspector/audit/audit-report.html:15
> +	   test(resolve, reject) {

Nit: use async test(

> LayoutTests/inspector/audit/audit-test-case.html:15
> +	   test(resolve, reject) {

This should be async test() in new code if possible.  >A>

> LayoutTests/inspector/audit/audit-test-case.html:17
> +	       InspectorTest.expectThat(auditTestCase instanceof
WI.AuditTestCase, "AuditTestCase is of instance test case.");

This assertion seems kind of pointless.

> LayoutTests/inspector/audit/audit-test-case.html:18
> +	       resolve();

>A>    this can be removed if async test()

> LayoutTests/inspector/audit/audit-test-case.html:23
> +	name: "Test functions must be asynchronous.",

Nit: weird spacing. Please use spaces instead of tabs, 4 spaces per indent.

> LayoutTests/inspector/audit/audit-test-case.html:25
> +	test(resolve, reject) {

Ditto re: async

> LayoutTests/inspector/audit/audit-test-case.html:26
> +			InspectorTest.expectException(() => {new
WI.AuditTestCase(new testSuiteFixture1, "fakeTest2", () => [])})

Nit: Please line wrap after the { and before }

> LayoutTests/inspector/audit/audit-test-suite.html:13
> +    let auditTestSuite2 = new testSuiteFixture1("FakeTestSuite",
"FakeTestSuite");

I think it's worth adding a test that makes sure that the first test case
finishes before the next one starts. async-test-suite.html has a fairly
exhaustive test suite that covers this sort of situation, so feel free to adapt
those to AuditTestCase / AuditManager internals. Rather than relying on correct
output, you can verify sequential execution directly. For example, you can pass
a local function as the test case body. Inside that function, it can verify no
other tests are in progress already by checking a global flag or something.

> LayoutTests/inspector/audit/audit-test-suite.html:18
> +	   test(resolve, reject) {

Ditto re: async

> LayoutTests/inspector/audit/audit-test-suite.html:19
> +	       InspectorTest.expectThat(typeof(auditTestSuite1.id) ===
"symbol", "AuditTestSuite1 has ID with correct type.");

Please use the appropriate variant of InspectorTest.expectEqual (in
TestHarness.js)

> LayoutTests/inspector/audit/audit-test-suite.html:29
> +	   test(resolve, reject) {

Ditto re: async

> LayoutTests/inspector/audit/audit-test-suite.html:30
> +	       InspectorTest.expectThat(auditTestSuite1.testCases.length === 2,
"There are two tests.");

Ditto re: expectEqual and friends

> LayoutTests/inspector/audit/resources/audit-test-fixtures.js:8
> +	      
this._buildTestCasesFromDescriptors(testSuiteFixture1.testCaseDescriptors());

See comment above, this should not need to be called manually by every subclass
in the common case.

> LayoutTests/inspector/audit/resources/audit-test-fixtures.js:9
> +	   }

Nit: needs newline

> LayoutTests/inspector/audit/resources/audit-test-fixtures.js:17
> +			   return Promise.resolve();

It's redundant to return a promise from an async function. You can just have an
empty body and it will resolve with the return value.

> LayoutTests/inspector/audit/resources/audit-test-fixtures.js:24
> +			   return Promise.reject([1, 2, 3, 4]);

Similarly, you can throw new Error([1,2,3,4]). What you have is slightly more
readable, so either way is fine.

> LayoutTests/inspector/audit/resources/audit-test-fixtures.js:37
> +	   }

Nit: needs extra newline

> LayoutTests/inspector/audit/resources/audit-test-fixtures.js:45
> +			   return Promise.resolve([]);

Ditto, this should be return []