1764717 - Add "ByType" variant for findMessage-related testing function and rewrite consumers to always specify message class

Status: RESOLVED FIXED

(DevTools :: Console, task)

Description

[Tooru Fujisawa [:arai]]

Derived from bug 1764682

bug 1764682 is caused by findMessage testing function returns console input, while the code should wait for console API output.

There seems to be similar situation in other tests, for the case that the test executes code from console and wait for output:

https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/devtools/client/webconsole/test/browser/browser_console_chrome_context_message.js#21-30

  const expectedMessages = [
    `Cu.reportError`, // bug 1561930
  ];

...
  execute(hud, `Cu.reportError("Cu.reportError");`); // bug 1561930
...
  await waitFor(() =>
    expectedMessages.every(expectedMessage => findMessage(hud, expectedMessage))

Those tests should pass special selector to the 3rd parameter of findMessage.
(such as ".console-api" for console.log, not sure for Cu.reportError, maybe just :not(.command) ?)

Or maybe we should make the default behavior ignore the console input (".message:not(.command)")?

Comment 1

[Tooru Fujisawa [:arai]]

testing patch that ignores command by default
https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=eed157b8cc37c6186a99e4d159a6d42e05ea8db2

there are some failure, that may be just that it's expecting command input, or perhaps some other issue.

Comment 2

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/mozilla-central/rev/150bf969aea0ec4434daccd86c52cc96f0f3c02a
The patch suggests that making the selector required parameter might be better.
or maybe provide separate function for ".message.command", ".message.result", etc

Comment 3

[Tooru Fujisawa [:arai]]

to avoid surprise by changing the function signature or parameter requirement,
maybe we could do this at the same time with bug 1669126, with single implementation across all tests, with possibly different function name.

there's already some functions in some shared-head.js, but some tests are still defining their own version.
https://searchfox.org/mozilla-central/search?q=function%20findMessage&path=

Comment 4

[Tooru Fujisawa [:arai]]

:nchevobbe, can I have your opinion here?

How do you think about making the selector parameter required?
Also, what do you think about adding new function (e.g. findMessageByClass) instead of modifying the existing function?

What I'm thinking right now is, introducing findMessageByClass(hud, "hello", ".console-api") and waitForMessageByClass(hud, "hello", ".error") etc.
The caller needs to pass the "message type"-specific class name, instead of the entire query, and internally add ".message" to generate the query.
(just to reduce the redundancy)

Those functions will be added to shared head.js file, and then replace each consumer with ByClass variants, fixing some unexpected cases (e.g. comment #0) at the same time, and once the replacement completes, remove findMessage functions where selector is optional,
so that new tests written from that point are required to specify class, and there's no new bug around finding/waiting for wrong message.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Tooru Fujisawa [:arai] from comment #4)

:nchevobbe, can I have your opinion here?

How do you think about making the selector parameter required?
Also, what do you think about adding new function (e.g. findMessageByClass) instead of modifying the existing function?

What I'm thinking right now is, introducing findMessageByClass(hud, "hello", ".console-api") and waitForMessageByClass(hud, "hello", ".error") etc.
The caller needs to pass the "message type"-specific class name, instead of the entire query, and internally add ".message" to generate the query.
(just to reduce the redundancy)

Those functions will be added to shared head.js file, and then replace each consumer with ByClass variants, fixing some unexpected cases (e.g. comment #0) at the same time, and once the replacement completes, remove findMessage functions where selector is optional,
so that new tests written from that point are required to specify class, and there's no new bug around finding/waiting for wrong message.

I like your proposal, overall it makes sense to force passing the classname so we don't get false positive.
We might introduce type-specific finder then (e.g. findEvaluationResultMessage, findErrorMessage, …) if we feel like callsites are a bit harder to read.

Comment 6

[Tooru Fujisawa [:arai]]

Thank you!
I'll work on it.

Comment 7

[Tooru Fujisawa [:arai]]

Just realized ByClass is misleading that the parameter is selector, not class name.
I'll go with ByType

Comment 8

[Tooru Fujisawa [:arai]]

I have question regarding findMessage(s)Virtualized vs findMessage(s).

findMessage(s)Virtualized seems to be used in very limited place, and most place uses findMessage(s).
Is it intentional, or all consumers are supposed to switch to findMessage(s)Virtualized ?
I'm wondering whether the new ByType functions should be based on findMessage(s)Virtualized algorithm or findMessage(s) algorithm.

Comment 9

[Tooru Fujisawa [:arai]]

here's WIP based on findMessage(s), that doesn't scroll.
https://treeherder.mozilla.org/jobs?repo=try&revision=b5091293640b8fe5abf7fa1a0ec573581c6b1263

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Tooru Fujisawa [:arai] from comment #8)

I have question regarding findMessage(s)Virtualized vs findMessage(s).

findMessage(s)Virtualized seems to be used in very limited place, and most place uses findMessage(s).
Is it intentional, or all consumers are supposed to switch to findMessage(s)Virtualized ?
I'm wondering whether the new ByType functions should be based on findMessage(s)Virtualized algorithm or findMessage(s) algorithm.

When we added virtualization, we added the findMessageVirtualized helper to use it in tests that were failing.
Lots of test didn't have to switch to the new helpers as they don't have enough message for the virtualization to kick in

So IMO the ByType function should be based on findMessage(s) , but maybe we could also have findMessage(s)VirtualizedByType at the same time (or later) ?

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 1: Add shared-head.js in devtools/client/webconsole/test/browser/_browser_console.ini. r?nchevobbe!

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 2: Define findMessageByType and findMessagesByType. r?nchevobbe!

Depends on D147021

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 3: Define type-specific wrappers for findMessageByType and findMessagesByType. r?nchevobbe!

Depends on D147022

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 4: Use type-specific findMessage(s) in devtools/client/webconsole/test/browser/browser_console_*. r?nchevobbe!

Depends on D147023

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 5: Use type-specific findMessage(s) in devtools/client/webconsole/test/browser/browser_jsterm_*. r?nchevobbe!

Depends on D147024

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 6: Use type-specific findMessage(s) in devtools/client/webconsole/test/browser/browser_webconsole_*. r?nchevobbe!

Depends on D147025

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 7: Define findAllMessages. r?nchevobbe!

Depends on D147026

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 8: Use findAllMessages in devtools/client/webconsole/test/browser/. r?nchevobbe!

Depends on D147027

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 9: Define findMessagePartByType and findMessagePartsByType. r?nchevobbe!

Depends on D147028

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 10: Use findMessagePart(s)ByType in devtools/client/webconsole/test/browser/. r?nchevobbe!

Depends on D147029

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 11: Add waitForRepeatedMessageByType. r?nchevobbe!

Depends on D147030

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 12: Use findMessageByType in testOpenInDebugger. r?nchevobbe!

Depends on D147031

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 13: Use findMessageByType in openMessageInNetmonitor. r?nchevobbe!

Depends on D147032

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 14: Use findErrorMessage in checkMessageStack. r?nchevobbe!

Depends on D147033

Comment 25

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 15: Remove findMessage and findMessages from devtools/client/webconsole/test/browser/head.js. r?nchevobbe!

Depends on D147034

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 16: Add findMessageVirtualizedByType and findMessagesVirtualizedByType. r?nchevobbe!

Depends on D147035

Comment 27

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 17: Use findMessageVirtualizedByType and findMessagesVirtualizedByType in tests. r?nchevobbe!

Depends on D147036

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 18: Use findMessagesVirtualizedByType in checkUniqueMessageExists. r?nchevobbe!

Depends on D147037

Comment 29

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 19: Use findMessageByType and findMessagesByType tests in other directories. r?nchevobbe!

Depends on D147038

Comment 30

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 20: Add waitForMessageByType and waitForMessagesByType. r?nchevobbe!

Depends on D147039

Comment 31

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 21: Use waitForMessageByType and waitForMessagesByType. r?nchevobbe!

Depends on D147040

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 22: Add executeAndWaitForMessageByType. r?nchevobbe!

Depends on D147041

Comment 33

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 23: Use executeAndWaitForMessageByType. r?nchevobbe!

Depends on D147042

Comment 34

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 24: Remove executeAndWaitForMessage. r?nchevobbe!

Depends on D147043

Comment 35

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 25: Change keyboardExecuteAndWaitForMessage to keyboardExecuteAndWaitForMessageByType. r?nchevobbe!

Depends on D147044

Comment 36

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 26: Remove waitForMessage and waitForMessages. r?nchevobbe!

Depends on D147045

Comment 37

[Tooru Fujisawa [:arai]]

Attachment: Bug 1764717 - Part 27: Convert remaining findMessageVirtualized to findMessageVirtualizedById. r?nchevobbe!

Depends on D147046

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04b84879d85e
Part 1: Add shared-head.js in devtools/client/webconsole/test/browser/_browser_console.ini. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/f35df94ea1d5
Part 2: Define findMessageByType and findMessagesByType. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/2a5382583807
Part 3: Define type-specific wrappers for findMessageByType and findMessagesByType. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/2abb293b94c8
Part 4: Use type-specific findMessage(s) in devtools/client/webconsole/test/browser/browser_console_*. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/df95e92908e0
Part 5: Use type-specific findMessage(s) in devtools/client/webconsole/test/browser/browser_jsterm_*. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/b8075ad20646
Part 6: Use type-specific findMessage(s) in devtools/client/webconsole/test/browser/browser_webconsole_*. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/189891fa3067
Part 7: Define findAllMessages. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/e96bc7b205dc
Part 8: Use findAllMessages in devtools/client/webconsole/test/browser/. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/15d1924a3ecf
Part 9: Define findMessagePartByType and findMessagePartsByType. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/6378c2ef157a
Part 10: Use findMessagePart(s)ByType in devtools/client/webconsole/test/browser/. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/1082127c4df2
Part 11: Add waitForRepeatedMessageByType. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/1ee9606f67eb
Part 12: Use findMessageByType in testOpenInDebugger. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/e83c3e63ca63
Part 13: Use findMessageByType in openMessageInNetmonitor. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/0f2347d0303f
Part 14: Use findErrorMessage in checkMessageStack. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/2975998b65ed
Part 15: Remove findMessage and findMessages from devtools/client/webconsole/test/browser/head.js. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/deff7858a2d5
Part 16: Add findMessageVirtualizedByType and findMessagesVirtualizedByType. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/099f223528fa
Part 17: Use findMessageVirtualizedByType and findMessagesVirtualizedByType in tests. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/ca07dc8f01b2
Part 18: Use findMessagesVirtualizedByType in checkUniqueMessageExists. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/861d0cd7b2fa
Part 19: Use findMessageByType and findMessagesByType tests in other directories. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/ef8b0bc412cb
Part 20: Add waitForMessageByType and waitForMessagesByType. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/94a81659571f
Part 21: Use waitForMessageByType and waitForMessagesByType. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/bc6db1de8b6f
Part 22: Add executeAndWaitForMessageByType. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/aef55b5eacb1
Part 23: Use executeAndWaitForMessageByType. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/385713053465
Part 24: Remove executeAndWaitForMessage. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/6c41fd93e6cb
Part 25: Change keyboardExecuteAndWaitForMessage to keyboardExecuteAndWaitForMessageByType. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/8a5c5a21ebe1
Part 26: Remove waitForMessage and waitForMessages. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/abb6eeffb44d
Part 27: Convert remaining findMessageVirtualized to findMessageVirtualizedById. r=nchevobbe

Comment 39

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/04b84879d85e
https://hg.mozilla.org/mozilla-central/rev/f35df94ea1d5
https://hg.mozilla.org/mozilla-central/rev/2a5382583807
https://hg.mozilla.org/mozilla-central/rev/2abb293b94c8
https://hg.mozilla.org/mozilla-central/rev/df95e92908e0
https://hg.mozilla.org/mozilla-central/rev/b8075ad20646
https://hg.mozilla.org/mozilla-central/rev/189891fa3067
https://hg.mozilla.org/mozilla-central/rev/e96bc7b205dc
https://hg.mozilla.org/mozilla-central/rev/15d1924a3ecf
https://hg.mozilla.org/mozilla-central/rev/6378c2ef157a
https://hg.mozilla.org/mozilla-central/rev/1082127c4df2
https://hg.mozilla.org/mozilla-central/rev/1ee9606f67eb
https://hg.mozilla.org/mozilla-central/rev/e83c3e63ca63
https://hg.mozilla.org/mozilla-central/rev/0f2347d0303f
https://hg.mozilla.org/mozilla-central/rev/2975998b65ed
https://hg.mozilla.org/mozilla-central/rev/deff7858a2d5
https://hg.mozilla.org/mozilla-central/rev/099f223528fa
https://hg.mozilla.org/mozilla-central/rev/ca07dc8f01b2
https://hg.mozilla.org/mozilla-central/rev/861d0cd7b2fa
https://hg.mozilla.org/mozilla-central/rev/ef8b0bc412cb
https://hg.mozilla.org/mozilla-central/rev/94a81659571f
https://hg.mozilla.org/mozilla-central/rev/bc6db1de8b6f
https://hg.mozilla.org/mozilla-central/rev/aef55b5eacb1
https://hg.mozilla.org/mozilla-central/rev/385713053465
https://hg.mozilla.org/mozilla-central/rev/6c41fd93e6cb
https://hg.mozilla.org/mozilla-central/rev/8a5c5a21ebe1
https://hg.mozilla.org/mozilla-central/rev/abb6eeffb44d