Bug 195265 - Web Inspector: Audit: there should be a centralized place for reusable code
Summary: Web Inspector: Audit: there should be a centralized place for reusable code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: WebInspectorAuditTab
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-03 21:27 PST by Devin Rousso
Modified: 2019-03-12 11:56 PDT (History)
11 users (show)

See Also:


Attachments
Patch (24.26 KB, patch)
2019-03-03 21:29 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (2.63 MB, application/zip)
2019-03-03 22:15 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.80 MB, application/zip)
2019-03-03 22:30 PST, EWS Watchlist
no flags Details
Patch (27.14 KB, patch)
2019-03-03 22:57 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.48 MB, application/zip)
2019-03-04 00:00 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.58 MB, application/zip)
2019-03-04 00:12 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (1.79 MB, application/zip)
2019-03-04 00:45 PST, EWS Watchlist
no flags Details
Patch (27.17 KB, patch)
2019-03-04 11:05 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.48 MB, application/zip)
2019-03-04 13:08 PST, EWS Watchlist
no flags Details
Patch (27.62 KB, patch)
2019-03-07 01:27 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-03-03 21:27:58 PST
As developers write their own audits, they will run into situations where they will want to reuse code.
Comment 1 Devin Rousso 2019-03-03 21:28:13 PST
<rdar://problem/47040673>
Comment 2 Devin Rousso 2019-03-03 21:29:58 PST
Created attachment 363484 [details]
Patch
Comment 3 Devin Rousso 2019-03-03 21:32:56 PST
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).
Comment 4 EWS Watchlist 2019-03-03 22:15:03 PST Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-03-03 22:15:04 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-03-03 22:30:41 PST Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-03-03 22:30:43 PST Comment hidden (obsolete)
Comment 8 Devin Rousso 2019-03-03 22:57:12 PST
Created attachment 363491 [details]
Patch
Comment 9 EWS Watchlist 2019-03-03 22:59:56 PST
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 10 EWS Watchlist 2019-03-04 00:00:55 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2019-03-04 00:00:57 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-03-04 00:12:52 PST Comment hidden (obsolete)
Comment 13 EWS Watchlist 2019-03-04 00:12:54 PST Comment hidden (obsolete)
Comment 14 EWS Watchlist 2019-03-04 00:45:33 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2019-03-04 00:45:35 PST Comment hidden (obsolete)
Comment 16 Devin Rousso 2019-03-04 11:05:24 PST
Created attachment 363529 [details]
Patch
Comment 17 EWS Watchlist 2019-03-04 13:08:37 PST Comment hidden (obsolete)
Comment 18 EWS Watchlist 2019-03-04 13:08:39 PST Comment hidden (obsolete)
Comment 19 Joseph Pecoraro 2019-03-06 20:35:31 PST
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?
Comment 20 Devin Rousso 2019-03-07 00:48:45 PST
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.
Comment 21 Devin Rousso 2019-03-07 01:27:26 PST
Created attachment 363856 [details]
Patch
Comment 22 WebKit Commit Bot 2019-03-12 11:56:34 PDT
Comment on attachment 363856 [details]
Patch

Clearing flags on attachment: 363856

Committed r242808: <https://trac.webkit.org/changeset/242808>
Comment 23 WebKit Commit Bot 2019-03-12 11:56:36 PDT
All reviewed patches have been landed.  Closing bug.