Bug 161278 – Web Inspector: Improve clarity of inspector tests by adding TestHarness.expect* functions similar to XCTest

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

TestHarness should provide a family of assertions/expectThat methods, similar to that provided by XCTest and other test frameworks (JUnit, NUnit, etc).

A quick audit of assert/expectThat usage in LayoutTests/inspector reveals the following usage patterns:

InspectorTest.assert(!expr)                          85
InspectorTest.assert(a === b)                       315
InspectorTest.assert(a !== b)                         1
InspectorTest.assert(a === null)                      4
InspectorTest.assert(Object.shallowEqual(a, b))       0

InspectorTest.expectThat(!expr)                      60
InspectorTest.expectThat(a === b)                   383
InspectorTest.expectThat(a !== b)                     8
InspectorTest.expectThat(a === null)                 11
InspectorTest.expectThat(Object.shallowEqual(a, b))  14

Based on this, TestHarness should include the following family of assertions (and corresponding expect* methods):

assert
assertFalse
assertNull
assertNotNull
assertEqual
assertNotEqual
assertShallowEqual

[Radar WebKit Bug Importer]

<rdar://problem/28039741>

[Matt Baker]

Created attachment 288538 [details]
[Patch] Proposed Fix

[Build Bot]

Comment on attachment 288538 [details]
[Patch] Proposed Fix

Attachment 288538 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2055983

New failing tests:
inspector/unit-tests/test-harness-expect-functions.html

[Build Bot]

Created attachment 288541 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288538 [details]
[Patch] Proposed Fix

Attachment 288538 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2056001

New failing tests:
inspector/unit-tests/test-harness-expect-functions.html

[Build Bot]

Created attachment 288542 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288538 [details]
[Patch] Proposed Fix

Attachment 288538 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2056019

New failing tests:
inspector/unit-tests/test-harness-expect-functions.html

[Build Bot]

Created attachment 288543 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Matt Baker]

Created attachment 288587 [details]
[Patch] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 288587 [details]
[Patch] Proposed Fix

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

r=me!

> Source/WebInspectorUI/UserInterface/Test/TestHarness.js:140
> +        console.assert(typeof expression1 === "number");
> +        console.assert(typeof expression2 === "number");

Are console.assert's useful in LayoutTests? Should we make these `this.assert(...)`?

> LayoutTests/inspector/unit-tests/test-harness-expect-functions.html:21
> +        args = args.map((a) => {
> +            if (typeof a === "number")
> +                return a;
> +            // Append empty string so `undefined` is displayed correctly.
> +            return JSON.stringify(a) + "";
> +        });
> +        return args.join(", ");

Nit: You can combine these lines:

    return args.map((a) => { ... }).join(", ");

> LayoutTests/inspector/unit-tests/test-harness-expect-functions.html:30
> +            test: () => {

Style: `test: () => {` => `test() {`

> LayoutTests/inspector/unit-tests/test-harness-expect-functions.html:33
> +                    if (!inputs || !inputs.length)
> +                        return;

Should probably output a fail message here, since this would be unexpected:

    InspectorTest.fail("Expected inputs.");

> LayoutTests/inspector/unit-tests/test-harness-expect-functions.html:59
> +    addInverseTestCase("expectFalse", expectThatTestCase);

I wonder if we should call this expectFalsey, though maybe more descriptive it would probably worse in code.

> LayoutTests/inspector/unit-tests/test-harness-expect-functions.html:72
> +    let expectEqualTestCase = {
> +        functionName: "expectEqual",
> +        passingInputs: [
> +            [true, true],

Lets add a case here for object equivalence: [object1, object1].

> LayoutTests/inspector/unit-tests/test-harness-expect-functions.html:79
> +            [true, false],

Same here for two different objects [object1, object2].

> LayoutTests/inspector/unit-tests/test-harness-expect-functions.html:101
> +            // Enable once <https://webkit.org/b/161852> is fixed.
> +            // [{}, []],

You should be able to enable this before landing.

[Matt Baker]

Comment on attachment 288587 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Test/TestHarness.js:140
>> +        console.assert(typeof expression2 === "number");
> 
> Are console.assert's useful in LayoutTests? Should we make these `this.assert(...)`?

It left like TestHarness.assert was intended for use in test code, not internally. FrontendTestHarness also uses console.assert.

However it looks like even after calling Inspector.debug(), console.assert (and debug) messages don't appear in test output. console.log/info/warn do.

[Matt Baker]

Created attachment 288601 [details]
[Patch] For Landing

[WebKit Commit Bot]

Comment on attachment 288601 [details]
[Patch] For Landing

Clearing flags on attachment: 288601

Committed r205834: <http://trac.webkit.org/changeset/205834>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.