Bug 195265 – Web Inspector: Audit: there should be a centralized place for reusable code

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

As developers write their own audits, they will run into situations where they will want to reuse code.

[Devin Rousso]

<rdar://problem/47040673>

[Devin Rousso]

Created attachment 363484 [details]
Patch

[Devin Rousso]

Comment on attachment 363484 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:106
> +    async setup()

I'm on the fence as to whether this should bump the `Audit.version`.  On one hand, it's new functionality that should be able to be feature-checked.  On the other hand, this is a purely additive change and won't affect any existing ability to run code.  I'm leaning towards bumping the version, as otherwise an audit written with the expectation of being able to use `setup` would most likely fail with many errors (e.g. functionality defined in the `setup` would be missing).

[EWS Watchlist]

Comment on attachment 363484 [details]
Patch

Attachment 363484 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11358610

New failing tests:
inspector/audit/teardown.html
inspector/audit/data-errors.html
inspector/audit/data-domAttributes.html
inspector/audit/run-dom.html
inspector/audit/data-domNodes.html
inspector/audit/run-accessibility.html
inspector/audit/basic.html

[EWS Watchlist]

Created attachment 363487 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363484 [details]
Patch

Attachment 363484 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11358722

New failing tests:
inspector/audit/teardown.html
inspector/audit/data-errors.html
inspector/audit/data-domAttributes.html
inspector/audit/data-domNodes.html
inspector/audit/run-accessibility.html
inspector/audit/basic.html

[EWS Watchlist]

Created attachment 363488 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 363491 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[EWS Watchlist]

Comment on attachment 363491 [details]
Patch

Attachment 363491 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11359368

New failing tests:
inspector/audit/data-domNodes.html
inspector/audit/data-domAttributes.html
inspector/audit/data-errors.html
inspector/audit/basic.html

[EWS Watchlist]

Created attachment 363494 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363491 [details]
Patch

Attachment 363491 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11359382

New failing tests:
inspector/audit/data-domNodes.html
inspector/audit/data-domAttributes.html
inspector/audit/data-errors.html
inspector/audit/basic.html

[EWS Watchlist]

Created attachment 363495 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363491 [details]
Patch

Attachment 363491 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11359440

New failing tests:
inspector/audit/data-domNodes.html
inspector/audit/data-domAttributes.html
inspector/audit/data-errors.html
inspector/audit/basic.html

[EWS Watchlist]

Created attachment 363498 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 363529 [details]
Patch

[EWS Watchlist]

Comment on attachment 363529 [details]
Patch

Attachment 363529 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11365188

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 363545 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Joseph Pecoraro]

Comment on attachment 363529 [details]
Patch

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

Awesome! r=me

> LayoutTests/inspector/audit/manager-start-setup.html:86
> +        description: "Check that Audit.run is able to respond with a RemoteObject.",

Some of these descriptions are a little generic given this is testing nested behavior.

> LayoutTests/inspector/audit/manager-start-setup.html:98
> +        async test() {
> +            const setupA = (function() {
> +                WebInspectorAudit.__test = "A";
> +            }).toString();
> +
> +            const setupB = (function() {
> +                WebInspectorAudit.__test = "B";
> +            }).toString();
> +
> +            let audit = new WI.AuditTestGroup("AuditManager.prototype.start.OverriddenLevelSetup.Group", [
> +                new WI.AuditTestCase("AuditManager.prototype.start.OverriddenLevelSetup.Test", auditTestString, {setup: setupA}),
> +            ], {setup: setupB});

Do I understand correctly that both of these setup's ran, and "A" ran first and was overridden by "B".

So our chain is only the Top Level setup, and the per-test Setup, but no additional groups in between?

[Devin Rousso]

Comment on attachment 363529 [details]
Patch

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

>> LayoutTests/inspector/audit/manager-start-setup.html:86
>> +        description: "Check that Audit.run is able to respond with a RemoteObject.",
> 
> Some of these descriptions are a little generic given this is testing nested behavior.

Copypasta :P  Will fix.

>> LayoutTests/inspector/audit/manager-start-setup.html:98
>> +            ], {setup: setupB});
> 
> Do I understand correctly that both of these setup's ran, and "A" ran first and was overridden by "B".
> 
> So our chain is only the Top Level setup, and the per-test Setup, but no additional groups in between?

Not quite.  This test is to make sure that we only run the top-level test.  What I really should be doing is adding to `WebInspectorAudit.__test` so that we can confirm that `"A"` never gets added to the string.  The idea is that only top-level `setup` are ever run.

[Devin Rousso]

Created attachment 363856 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 363856 [details]
Patch

Clearing flags on attachment: 363856

Committed r242808: <https://trac.webkit.org/changeset/242808>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.