180831 – Web Inspector: InspectorTest.evaluateInPage should unwrap primitive values by default

RESOLVED FIXED 180831

Web Inspector: InspectorTest.evaluateInPage should unwrap primitive values by default

https://bugs.webkit.org/show_bug.cgi?id=180831

Summary Web Inspector: InspectorTest.evaluateInPage should unwrap primitive values by...

Blaze Burg

Reported 2017-12-14 13:03:56 PST

This makes tests shorter and nicer.

Attachments
Proposed Fix (46.78 KB, patch) 2017-12-14 13:09 PST, Blaze Burg	no flags	Details Formatted Diff Diff
For Landing (46.21 KB, patch) 2017-12-14 15:59 PST, Blaze Burg	no flags	Details Formatted Diff Diff
For Landing (45.27 KB, patch) 2017-12-14 16:23 PST, Blaze Burg	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Blaze Burg

Comment 1 2017-12-14 13:09:19 PST

Created attachment 329388 [details] Proposed Fix

Joseph Pecoraro

Comment 2 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.

Blaze Burg

Comment 3 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.

Blaze Burg

Comment 4 2017-12-14 15:59:02 PST

Created attachment 329414 [details] For Landing

Joseph Pecoraro

Comment 5 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.

Blaze Burg

Comment 6 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!

Blaze Burg

Comment 7 2017-12-14 16:23:34 PST

Created attachment 329416 [details] For Landing

WebKit Commit Bot

Comment 8 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>

WebKit Commit Bot

Comment 9 2017-12-14 17:39:17 PST

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 10 2017-12-14 17:41:15 PST

<rdar://problem/36063909>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Blaze Burg

Reported

2017-12-14 13:03 PST

Modified

2017-12-14 17:41 PST History

CC List

5 users Show

URL

Keywords InRadar

Depends on

Blocks