Bug 180831 - Web Inspector: InspectorTest.evaluateInPage should unwrap primitive values by default
Summary: Web Inspector: InspectorTest.evaluateInPage should unwrap primitive values by...
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: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-14 13:03 PST by BJ Burg
Modified: 2017-12-14 17:41 PST (History)
5 users (show)

See Also:


Attachments
Proposed Fix (46.78 KB, patch)
2017-12-14 13:09 PST, BJ Burg
no flags Details | Formatted Diff | Diff
For Landing (46.21 KB, patch)
2017-12-14 15:59 PST, BJ Burg
no flags Details | Formatted Diff | Diff
For Landing (45.27 KB, patch)
2017-12-14 16:23 PST, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-12-14 13:03:56 PST
This makes tests shorter and nicer.
Comment 1 BJ Burg 2017-12-14 13:09:19 PST
Created attachment 329388 [details]
Proposed Fix
Comment 2 Joseph Pecoraro 2017-12-14 15:10:39 PST
Comment on attachment 329388 [details]
Proposed Fix

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

r=me

> LayoutTests/inspector/console/command-line-api-copy.html:27
> +            InspectorTest.evaluateInPage("pasteAndReturnString()", (error, result) => {

Nit: Lets update the source string to be a template string: `pasteAndReturnString()`.

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page-expected.txt:165
> +PASS: Expected and actual evaluation result type should be equal.
> +PASS: Expected and actual evaluation result subtype should be equal.

These lines are a bit vague, but I guess if it fails it outputs the two types? I normally like to see the expected type in the output so I don't have to search the test code for what it was.

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:1
> +<!doctype html>

Nice test!

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:10
> +            input: "-42",

Style: I find it clearer when a string is intended to be code, to use template strings. `-42` makes it just a little bit clearer that this will be evaluated later on.

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:143
> +            for (let {input, type, subtype, thrown} of complexCases) {
> +                let result = await InspectorTest.evaluateInPage(input);

Technically this is serial (request, response, request, response, ...) but you could do this all in parallel (request1, request2, ..., response1, response2, ...) like so:

    let results = await Promise.all(...complexCases.map((input) => InspectorTest.evaluateInPage(input))];
    for (let {input, type, subtype, thrown} of results)

But I don't think it matters if the debug bugs aren't timing out.

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:146
> +                InspectorTest.expectThat(result instanceof WI.RemoteObject, "Returned result should be a WI.RemoteObject.");

Maybe we should really have `expectInstanceOf` because we do this all over the place.

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:150
> +                InspectorTest.log("\n");

Nit: Just `InspectorTest.log("");` will put 1 newline between things instead of 2.
Comment 3 BJ Burg 2017-12-14 15:18:25 PST
(In reply to Joseph Pecoraro from comment #2)
> Comment on attachment 329388 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329388&action=review
> 
> r=me
> 
> > LayoutTests/inspector/console/command-line-api-copy.html:27
> > +            InspectorTest.evaluateInPage("pasteAndReturnString()", (error, result) => {
> 
> Nit: Lets update the source string to be a template string:
> `pasteAndReturnString()`.

ok.
 
> > LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page-expected.txt:165
> > +PASS: Expected and actual evaluation result type should be equal.
> > +PASS: Expected and actual evaluation result subtype should be equal.
> 
> These lines are a bit vague, but I guess if it fails it outputs the two
> types? I normally like to see the expected type in the output so I don't
> have to search the test code for what it was.

It does output the two types, and at least the input string is being printed.


> > LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:1
> > +<!doctype html>
> 
> Nice test!
> 
> > LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:10
> > +            input: "-42",
> 
> Style: I find it clearer when a string is intended to be code, to use
> template strings. `-42` makes it just a little bit clearer that this will be
> evaluated later on.

Ok.

> 
> > LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:143
> > +            for (let {input, type, subtype, thrown} of complexCases) {
> > +                let result = await InspectorTest.evaluateInPage(input);
> 
> Technically this is serial (request, response, request, response, ...) but
> you could do this all in parallel (request1, request2, ..., response1,
> response2, ...) like so:
> 
>     let results = await Promise.all(...complexCases.map((input) =>
> InspectorTest.evaluateInPage(input))];
>     for (let {input, type, subtype, thrown} of results)
> 
> But I don't think it matters if the debug bugs aren't timing out.

I'll leave it as-is unless needed.
 
> > LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:146
> > +                InspectorTest.expectThat(result instanceof WI.RemoteObject, "Returned result should be a WI.RemoteObject.");
> 
> Maybe we should really have `expectInstanceOf` because we do this all over
> the place.

Good idea.
 
> > LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:150
> > +                InspectorTest.log("\n");
> 
> Nit: Just `InspectorTest.log("");` will put 1 newline between things instead
> of 2.

Oops, thanks. I noticed that passing no arguments will log undefined.
Comment 4 BJ Burg 2017-12-14 15:59:02 PST
Created attachment 329414 [details]
For Landing
Comment 5 Joseph Pecoraro 2017-12-14 16:03:38 PST
Comment on attachment 329414 [details]
For Landing

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

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page-expected.txt:41
> +FAIL: Expected and actual evaluation result should be equal.
> +    Expected: -42
> +    Actual: RemoteObject instance #1

Fails here. Seems we need to flip something.
Comment 6 BJ Burg 2017-12-14 16:21:56 PST
(In reply to Joseph Pecoraro from comment #5)
> Comment on attachment 329414 [details]
> For Landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329414&action=review
> 
> > LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page-expected.txt:41
> > +FAIL: Expected and actual evaluation result should be equal.
> > +    Expected: -42
> > +    Actual: RemoteObject instance #1
> 
> Fails here. Seems we need to flip something.

Accidentally moved around this assertion into this test case where it isn't valid. Oops. Good catch!
Comment 7 BJ Burg 2017-12-14 16:23:34 PST
Created attachment 329416 [details]
For Landing
Comment 8 WebKit Commit Bot 2017-12-14 17:39:15 PST
Comment on attachment 329416 [details]
For Landing

Clearing flags on attachment: 329416

Committed r225950: <https://trac.webkit.org/changeset/225950>
Comment 9 WebKit Commit Bot 2017-12-14 17:39:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2017-12-14 17:41:15 PST
<rdar://problem/36063909>