Bug 192171 - Web Inspector: Audit: tests should support async operations
Summary: Web Inspector: Audit: tests should support async operations
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: 2018-11-29 12:19 PST by Devin Rousso
Modified: 2018-12-04 01:08 PST (History)
15 users (show)

See Also:


Attachments
Patch (45.33 KB, patch)
2018-11-29 12:27 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.57 MB, application/zip)
2018-11-29 13:28 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.15 MB, application/zip)
2018-11-29 13:42 PST, EWS Watchlist
no flags Details
Patch (46.88 KB, patch)
2018-11-29 13:57 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.99 MB, application/zip)
2018-11-29 15:20 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews103 for mac-sierra (2.65 MB, application/zip)
2018-11-29 15:43 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (2.30 MB, application/zip)
2018-11-29 17:01 PST, EWS Watchlist
no flags Details
Patch (47.37 KB, patch)
2018-11-30 00:56 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.46 MB, application/zip)
2018-11-30 01:38 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (1.99 MB, application/zip)
2018-11-30 02:32 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.73 MB, application/zip)
2018-11-30 09:12 PST, EWS Watchlist
no flags Details
Patch (47.79 KB, patch)
2018-11-30 11:19 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] Mapped Callbacks (48.22 KB, patch)
2018-11-30 12:08 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (52.09 KB, patch)
2018-11-30 12:32 PST, Devin Rousso
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.24 MB, application/zip)
2018-11-30 13:45 PST, EWS Watchlist
no flags Details
Patch (51.26 KB, patch)
2018-11-30 13:50 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (51.26 KB, patch)
2018-11-30 20:26 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (62.14 KB, patch)
2018-12-03 20:55 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (62.34 KB, patch)
2018-12-03 21:24 PST, Devin Rousso
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.47 MB, application/zip)
2018-12-03 22:26 PST, EWS Watchlist
no flags Details
Patch (62.34 KB, patch)
2018-12-03 22:28 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 2018-11-29 12:19:43 PST
If a test returns a Promise (either by manually `return new Promise(...)` or using an `async function() { ... }`), we should wait on that promise before completing the test.
Comment 1 Devin Rousso 2018-11-29 12:27:07 PST
Created attachment 356034 [details]
Patch
Comment 2 EWS Watchlist 2018-11-29 12:29:18 PST Comment hidden (obsolete)
Comment 3 EWS Watchlist 2018-11-29 12:29:29 PST Comment hidden (obsolete)
Comment 4 EWS Watchlist 2018-11-29 13:28:41 PST Comment hidden (obsolete)
Comment 5 EWS Watchlist 2018-11-29 13:28:43 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-11-29 13:42:28 PST Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-11-29 13:42:30 PST Comment hidden (obsolete)
Comment 8 Devin Rousso 2018-11-29 13:57:22 PST
Created attachment 356056 [details]
Patch

Add locking
Comment 9 EWS Watchlist 2018-11-29 13:59:49 PST Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-11-29 15:20:27 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2018-11-29 15:20:29 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2018-11-29 15:43:32 PST Comment hidden (obsolete)
Comment 13 EWS Watchlist 2018-11-29 15:43:33 PST Comment hidden (obsolete)
Comment 14 EWS Watchlist 2018-11-29 17:01:24 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-11-29 17:01:26 PST Comment hidden (obsolete)
Comment 16 Devin Rousso 2018-11-30 00:56:00 PST
Created attachment 356160 [details]
Patch
Comment 17 EWS Watchlist 2018-11-30 01:00:18 PST Comment hidden (obsolete)
Comment 18 EWS Watchlist 2018-11-30 01:38:04 PST Comment hidden (obsolete)
Comment 19 EWS Watchlist 2018-11-30 01:38:06 PST Comment hidden (obsolete)
Comment 20 EWS Watchlist 2018-11-30 02:32:36 PST Comment hidden (obsolete)
Comment 21 EWS Watchlist 2018-11-30 02:32:38 PST Comment hidden (obsolete)
Comment 22 EWS Watchlist 2018-11-30 09:12:19 PST Comment hidden (obsolete)
Comment 23 EWS Watchlist 2018-11-30 09:12:20 PST Comment hidden (obsolete)
Comment 24 Devin Rousso 2018-11-30 11:19:55 PST
Created attachment 356200 [details]
Patch
Comment 25 EWS Watchlist 2018-11-30 11:23:16 PST Comment hidden (obsolete)
Comment 26 Joseph Pecoraro 2018-11-30 11:56:42 PST
Comment on attachment 356160 [details]
Patch

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

> Source/JavaScriptCore/inspector/InjectedScriptBase.cpp:131
> +    ASSERT(!hadException);
> +    // FIXME: since `callback` is moved above, we can't call it if there's an execption while trying
> +    //        to execute the `JSNativeStdFunction` inside InjectedScriptSource.js.

I think it is fine to just ASSERT here. But you can invoke the JSNativeStdFunction yourself (or reach into it and call it). Just save a reference to it yourself above.

> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:39
> +    static supportsAwaitPromise()

Can we just inline this, with a

    // COMPATIBILITY (iOS 12): ...

> LayoutTests/inspector/runtime/awaitPromise.html:32
> +    function addValueTest(name, value) {
> +        addTest(name, `Promise.resolve(${JSON.stringify(value)})`, {returnByValue: true, saveResult: true}, (remoteObject, wasThrown) => {

There should be tests for a rejected promise. There are only Tests here for resolved promises.
Comment 27 Devin Rousso 2018-11-30 12:08:34 PST
Created attachment 356218 [details]
[Patch] Mapped Callbacks

Store callbacks within a map on InjectedScriptBase so that they can later be retrieved
Comment 28 EWS Watchlist 2018-11-30 12:11:01 PST Comment hidden (obsolete)
Comment 29 Devin Rousso 2018-11-30 12:32:02 PST
Created attachment 356220 [details]
Patch
Comment 30 EWS Watchlist 2018-11-30 12:34:42 PST Comment hidden (obsolete)
Comment 31 EWS Watchlist 2018-11-30 13:45:56 PST Comment hidden (obsolete)
Comment 32 EWS Watchlist 2018-11-30 13:45:58 PST Comment hidden (obsolete)
Comment 33 Devin Rousso 2018-11-30 13:50:14 PST
Created attachment 356229 [details]
Patch
Comment 34 Devin Rousso 2018-11-30 20:26:35 PST
Created attachment 356286 [details]
Patch

This is what happens when you manually edit a diff... :|
Comment 35 EWS Watchlist 2018-11-30 20:29:22 PST Comment hidden (obsolete)
Comment 36 Radar WebKit Bug Importer 2018-12-03 10:41:01 PST
<rdar://problem/46423562>
Comment 37 Joseph Pecoraro 2018-12-03 18:54:50 PST
Comment on attachment 356286 [details]
Patch

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

r=me

> Source/JavaScriptCore/inspector/InjectedScriptBase.cpp:138
> +        // Since `callback` is moved above, we can't call it if there's an execption while trying to

Typo: "execption"

> Source/JavaScriptCore/inspector/protocol/Runtime.json:230
> +            "description": "Evaluates promise on global object.",

Might use a better description. This calls the callback when the given objectId (which must be a Promise) resolves / rejects.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:239
> +            let response = null;
> +
> +            metadata.startTimestamp = new Date;
> +            response = await RuntimeAgent.evaluate.invoke(evaluateArguments);

Merge the `let response = ...` here?

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:244
> +                    metadata.asyncTimestamp = metadata.endTimestamp;

Note to Devin: Add `asyncTimestamp` to `fromPayload` (so export/import uses it).

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:249
> +                    addError(WI.UIString("Async audits are not supported on this device."));

Probably better to just say "are not support". The "on this device" thing is a little weird.

> LayoutTests/inspector/runtime/awaitPromise.html:49
> +        let expression = `new Promise((resolve, reject) => setTimeout(reject, 100, ${JSON.stringify(value)}))`;

If this is triggering onunhandledrejection console messages we should provide a way in tests to mute those so that this doesn't end up being flakey.

> LayoutTests/inspector/runtime/awaitPromise.html:77
> +    addRejectTest("Runtime.awaitPromise.Reject.Object", {a: 1, b: 2});

I'd also want to see a Promise chain.

    • Promise (1) that resolves to
        -> Promise (2) that resolves to
            -> Promise (3) that resolves to
                -> a value

Should end up with the final value and not object (2).
Comment 38 Devin Rousso 2018-12-03 20:55:08 PST
Created attachment 356453 [details]
Patch
Comment 39 EWS Watchlist 2018-12-03 21:00:47 PST Comment hidden (obsolete)
Comment 40 Devin Rousso 2018-12-03 21:24:42 PST
Created attachment 356457 [details]
Patch
Comment 41 EWS Watchlist 2018-12-03 21:28:56 PST Comment hidden (obsolete)
Comment 42 EWS Watchlist 2018-12-03 22:26:10 PST Comment hidden (obsolete)
Comment 43 EWS Watchlist 2018-12-03 22:26:12 PST Comment hidden (obsolete)
Comment 44 Devin Rousso 2018-12-03 22:28:42 PST
Created attachment 356471 [details]
Patch
Comment 45 EWS Watchlist 2018-12-03 22:40:21 PST Comment hidden (obsolete)
Comment 46 WebKit Commit Bot 2018-12-04 01:08:51 PST
Comment on attachment 356471 [details]
Patch

Clearing flags on attachment: 356471

Committed r238850: <https://trac.webkit.org/changeset/238850>
Comment 47 WebKit Commit Bot 2018-12-04 01:08:53 PST
All reviewed patches have been landed.  Closing bug.