Bug 180831

Summary:

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

Product:

WebKit

Reporter:

Blaze Burg <bburg>

Component:

Web Inspector

Assignee:

Blaze Burg <bburg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, commit-queue, inspector-bugzilla-changes, joepeck, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Proposed Fix	none
For Landing	none
For Landing	none

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.