Bug 184071 – AX: Audit Tab should have an Audit Manager

• Component: Accessibility
• Status: RESOLVED FIXED

[Aaron Chu]

This patch only contains a proposal for the audit manager and does not contain all the necessary models and views.

[Radar WebKit Bug Importer]

<rdar://problem/38946364>

[Radar WebKit Bug Importer]

<rdar://problem/38946365>

[Aaron Chu]

Created attachment 336647 [details]
Patch

[Blaze Burg]

Comment on attachment 336647 [details]
Patch

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

r-. I can't really review this because there are a lot of missing class references, like WI.AuditTestCase, WI.AuditTestSuite, etc. These should all go into the same patch.

The manager should not be firing events that are state changes of the model object themselves (i.e., a suite or test case). As written this would be really annoying to test because the event logic is all intertwined. The manager should be keeping a list of reports, test cases, and suites; support creating a new report from a suite; and provide other functionality like import/export.

> Source/WebInspectorUI/ChangeLog:23
> [phone]  Aaron Chu  <[e-mail]>

Nit: double ChangeLog.

[Blaze Burg]

Also, some very basic unit tests that instantiate/run a basic set of test cases and suites would be good to flush out problems in the code.

[Aaron Chu]

Created attachment 337600 [details]
Patch

[Aaron Chu]

Created attachment 338198 [details]
Patch

[Blaze Burg]

Comment on attachment 338198 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:9
> +        AuditManager and models used in Audit feature.

You really should describe the overall design here. Is a report generated from a test case? Does the manager own the reports or does the test case? What's the reason for an audit report to be mutable while the client has a reference to it?

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:34
> +        this._reports = {};

This should be a Map.

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:35
> +        this._reportKeys = [];

This is redundant with Object.keys() or a key iterator for a Map.

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:37
> +        this._createTestSuites();

This won't do anything..?

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:43
> +    get testSuites() { return this._testSuites; }

In JS, it's by convention that we return a copy of an internal data structure to avoid clients from screwing up our data.

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:44
> +    get reports() { return this._reports; }

ditto.

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:49
> +        if (representedObject instanceof WI.AuditTestCase)

This function has me puzzled. It looks like the test suite code path will not actually return a report because resultData will be undefined. performTestOnTestSuite() doesn't seem to return a promise either.

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:77
> +        this._reportKeys.forEach(reportId => {

This function does not need to exist if a Map is used.

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:84
> +    _addReport(auditReport)

This is only used in one place. Please inline into _generateReport.

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:98
> +        this._testSuites = this._testSuiteConstructors.map(suite =>  {

I don't understand.

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:117
> +        if (event.data.isLastResult) {

I'd prefer to negate the condition and make it an early return.

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:124
> +    {

This function can be removed when this._reports is a Map.

> Source/WebInspectorUI/UserInterface/Audit/Controllers/AuditManager.js:133
> +        delete this._reports[reportId];

This function can be removed when this._reports is a Map.

> Source/WebInspectorUI/UserInterface/Audit/Models/AuditReport.js:71
> +        return "passed";

This seems like a mistake?

> Source/WebInspectorUI/UserInterface/Audit/Models/AuditReport.js:76
> +        if (!this.failed)

This method does not belong in a model class. Either the controller that saves it to file, or the view object, should be generating the title.

> Source/WebInspectorUI/UserInterface/Audit/Models/AuditReport.js:114
> +        if (result.failed) {

This is going to sort for every append to the results. Why is this desirable (even without the perf problem)?

I don't quite understand why the class maintains separate results arrays per log level. It would be better to store one big array, and accessors can create filtered copies from that. We'd need to create a copy of the results to hand out anyway since this class is not immutable.

> Source/WebInspectorUI/UserInterface/Audit/Models/AuditResult.js:32
> +        this._nodeIds = data.nodeIds;

What is this for?

> Source/WebInspectorUI/UserInterface/Audit/Models/AuditResult.js:33
> +        this._isPassed = !this._nodeIds.length;

Nit: don't start members with _is. Just this._passed is fine.

> Source/WebInspectorUI/UserInterface/Audit/Models/AuditResult.js:53
> +        return {

I don't understand what this is for.

> Source/WebInspectorUI/UserInterface/Audit/Models/AuditResult.js:61
> +    get title()

Ditto.

> Source/WebInspectorUI/UserInterface/Audit/Models/AuditTestCase.js:64
> +        for (let item in resultData)

What is going on here?

> Source/WebInspectorUI/UserInterface/Audit/Models/AuditTestSuite.js:66
> +        this.testCaseIds.forEach(async (testCaseId, index) => {

This method should return a promise. Actually, why can't it return the audit report itself? I'm also not sure if forEach works with async callbacks.. and error handling seems missing. It would be more natural to use Promise.all() in this situation. If you really need these to run serially, consider copying some of the promise-chaining code from AsyncTestSuite.

> Source/WebInspectorUI/UserInterface/Audit/Models/AuditTestSuite.js:87
> +            test = test || function(){return};

I don't think we should accept empty test functions. I would also argue strongly in favor of only allowing async functions as test functions. You can check this like so:

test[Symbol.toStringTag] === "AsyncFunction"

> Source/WebInspectorUI/UserInterface/Main.html:453
> +    <script src="Audit/Models/AuditReport.js"></script>

These files should not be in their own top level directory.

> LayoutTests/inspector/audit/audit-test-suite.html:63
> +                InspectorTest.expectThat(result.auditResult instanceof WI.AuditResult && typeof(result.testSuiteId) === "symbol", "Result is Valid.");

This should be two assertions.

[Aaron Chu]

Created attachment 342727 [details]
Patch

[Blaze Burg]

Comment on attachment 342727 [details]
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 []

[Aaron Chu]

Comment on attachment 342727 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:109
>> +        this._testSuiteConstructors.push(auditTestSuiteConstructor);
> 
> Why do we keep instances and constructors both?

My thought was that if a new test suite was added, we can compare the constructors before we instantiate.

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

errorDetails is an object that contains the meta information (ex. the title,  error code, log level etc…) for an error. It is defined in the descriptor and is instantiated with an AuditTestCase. An AuditResult takes this information from the AuditTestCase instance, and can pass this information along to the View.

[Aaron Chu]

Comment on attachment 342727 [details]
Patch

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

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

I used `slice(1)` to reference the `testCases[1]`here because I provided the `testCases[0]` as the second parameter for `reduce()`. I wanted the reduction to start  immediately at index `testCases[0]` and `testCases[1]` instead of with `Promise.resolve` and testCases[0].  Removing `1` from `slice()` would causes `testCase[0]` to run twice. Starting with `Promise.resolve` would fail an assertion in WI.AuditResult that expects to received an instance of Wi.AuditResult.

[Aaron Chu]

Created attachment 344826 [details]
Patch

[EWS Watchlist]

Comment on attachment 344826 [details]
Patch

Attachment 344826 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8512567

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 344830 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Aaron Chu]

(In reply to Build Bot from comment #15)
> Created attachment 344830 [details]
> Archive of layout-test-results from ews205 for win-future
> 
> The attached test failures were seen while running run-webkit-tests on the
> win-ews.
> Bot: ews205  Port: win-future  Platform:
> CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Not sure if this is related to the patch. None of the objects proposed in this patch touches on the domains that failed here.

[Aaron Chu]

(In reply to Build Bot from comment #15)
> Created attachment 344830 [details]
> Archive of layout-test-results from ews205 for win-future
> 
> The attached test failures were seen while running run-webkit-tests on the
> win-ews.
> Bot: ews205  Port: win-future  Platform:
> CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

ditto

[Aaron Chu]

The iOS build failure seems to have something to do with SSL and/or the read operation.

[Blaze Burg]

Comment on attachment 344826 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:38
> +            if (newTestSuite instanceof WI.AuditTestSuite)

This is a programming error if suite is of the wrong instance, but as written this will fill in `undefined` elements into this._testSuites. You should throw an exception in this case instead.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:47
> +        return [...this._reports.values()];

Newlines not necessary.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:87
> +            // Making AuditReport read-only after all the AuditResults have been received.

Nit: 'Make'

> Source/WebInspectorUI/UserInterface/Main.html:865
> +    <script src="Controllers/AuditManager.js"></script>

Nit: please alphabetize this if it does not really need to be ordered after these other files.

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

This returns an array but the getter sounds like its singular. Please make it plural?

> Source/WebInspectorUI/UserInterface/Models/AuditReport.js:67
> +        return this._results.slice().filter(result => result.failed);

Unless you are using this elsewhere, please just inline it into get failedCount().

> Source/WebInspectorUI/UserInterface/Test.html:231
>  

Ditto the above comment.

> LayoutTests/inspector/audit/audit-manager.html:37
> +            auditManager.addTestSuite(testSuiteFixture1);

I would prefer that this throw an exception and not be allowed, as it seems to only be possible as a programming error.

> LayoutTests/inspector/audit/audit-report.html:5
> +<script src="./resources/audit-test-fixtures.js"></script>

Nit: don't need the leading ./

> LayoutTests/inspector/audit/audit-report.html:9
> +    

Nit: extra newline

> LayoutTests/inspector/audit/audit-test-case.html:9
> +

Ditto.

[Aaron Chu]

Created attachment 345010 [details]
Patch

[EWS Watchlist]

Comment on attachment 345010 [details]
Patch

Attachment 345010 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8532607

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html

[EWS Watchlist]

Created attachment 345015 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Aaron Chu]

(In reply to Build Bot from comment #21)
> Comment on attachment 345010 [details]
> Patch
> 
> Attachment 345010 [details] did not pass win-ews (win):
> Output: https://webkit-queues.webkit.org/results/8532607
> 
> New failing tests:
> http/tests/security/canvas-remote-read-remote-video-localhost.html

unrelated to patch.

[Aaron Chu]

(In reply to Build Bot from comment #22)
> Created attachment 345015 [details]
> Archive of layout-test-results from ews206 for win-future
> 
> The attached test failures were seen while running run-webkit-tests on the
> win-ews.
> Bot: ews206  Port: win-future  Platform:
> CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

ditto.

[WebKit Commit Bot]

Comment on attachment 345010 [details]
Patch

Clearing flags on attachment: 345010

Committed r233858: <https://trac.webkit.org/changeset/233858>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.