Bug 161113 – Web Inspector: Modernize inspector/indexeddb tests

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

[Joseph Pecoraro]

Modernize inspector/indexeddb tests.

They are written in an old, hard to follow style. Modernizing these will make the test and output much easier to read.

[Joseph Pecoraro]

Created attachment 286795 [details]
[PATCH] Proposed Fix

[Blaze Burg]

Comment on attachment 286795 [details]
[PATCH] Proposed Fix

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

r- because I think the evaluateInPage in one test case can race with the next test starting. Otherwise this looks like good cleanup and reminded me of a few cleanups we can do to make tests nicer.

> LayoutTests/inspector/indexeddb/deleteDatabaseNamesWithSpace.html:10
> +        event.target.result.close();

I would alias this just to name the result type.

> LayoutTests/inspector/indexeddb/deleteDatabaseNamesWithSpace.html:29
> +            IndexedDBAgent.requestDatabaseNames(WebInspector.frameResourceManager.mainFrame.securityOrigin, (error, names) => {

If you are feeling adventurous, you can use the returned promise.

IndexedDBAgent.requestDatabaseNames(WebInspector.frameResourceManager.mainFrame.securityOrigin)
.then((names) => { ... })
.then(resolve).catch(reject);

Inside AsyncTestSuite, it will log anything (string, object, Error) passed to the reject callback.

I think the .then(resolve).catch(reject) is preferable to .then(resolve, reject) as the verbs make it easier to follow. They are semantically the same thing though, as a missing second argument to then() is treated as a fall-through for a rejected promise.

> LayoutTests/inspector/indexeddb/deleteDatabaseNamesWithSpace.html:30
> +                InspectorTest.evaluateInPage("deleteDatabaseNames(" + JSON.stringify(names) + ")");

This is not a synchronous call so this will race with the next test case. Unfortunately the InspectorTest layer only support a callback version, so you would need to pass resolve as the second argument.

> LayoutTests/inspector/indexeddb/deleteDatabaseNamesWithSpace.html:41
> +                InspectorTest.expectNoError(error);

... so expectNoError would not be necessary in this test case if it were written with promises as suggested above.

> LayoutTests/inspector/indexeddb/deleteDatabaseNamesWithSpace.html:52
> +            InspectorTest.evaluateInPage("createDatabase('Database With Space')");

Same comment as above, this could race in general (but not here as the two backend calls will be sequentially ordered).

> LayoutTests/inspector/indexeddb/deleteDatabaseNamesWithSpace.html:55
> +                InspectorTest.expectThat(names.length === 1, "A single IndexedDB database should exist.");

Please add a FIXME for InspectorTest.expectEquals.

> LayoutTests/inspector/indexeddb/requestDatabaseNames.html:10
> +        event.target.result.close();

Same comment as above re: aliasing.

[Blaze Burg]

Comment on attachment 286795 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/indexeddb/deleteDatabaseNamesWithSpace.html:30
>> +                InspectorTest.evaluateInPage("deleteDatabaseNames(" + JSON.stringify(names) + ")");
> 
> This is not a synchronous call so this will race with the next test case. Unfortunately the InspectorTest layer only support a callback version, so you would need to pass resolve as the second argument.

After further discussion, this specific case is fine. In general, this could be problematic if model objects being accessed are updated via events, in that case it's better to wait for a model update event before proceeding.

[Joseph Pecoraro]

Comment on attachment 286795 [details]
[PATCH] Proposed Fix

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

>>> LayoutTests/inspector/indexeddb/deleteDatabaseNamesWithSpace.html:30
>>> +                InspectorTest.evaluateInPage("deleteDatabaseNames(" + JSON.stringify(names) + ")");
>> 
>> This is not a synchronous call so this will race with the next test case. Unfortunately the InspectorTest layer only support a callback version, so you would need to pass resolve as the second argument.
> 
> After further discussion, this specific case is fine. In general, this could be problematic if model objects being accessed are updated via events, in that case it's better to wait for a model update event before proceeding.

If I understand you correctly, the issue you see here is that this individual TestCase is not totally complete before the next TestCase runs. I can change this.

>> LayoutTests/inspector/indexeddb/deleteDatabaseNamesWithSpace.html:55
>> +                InspectorTest.expectThat(names.length === 1, "A single IndexedDB database should exist.");
> 
> Please add a FIXME for InspectorTest.expectEquals.

I don't like the idea of an equals here. Logging the output is succinct, we see the output in the test so we will immediately know if something changes to it without having to update multiple places.

In the later tests though, there would totally be a use case for an expectEquals.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/205039>