1788437 - Use pprint for assertion messages

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P3)

Description

[Julian Descottes [:jdescottes]]

From https://phabricator.services.mozilla.com/D155522#inline-858885

Just noticed that for all the assertions we might actually want to use pprint so that the type is printed as well. Otherwise it might be hard to see what's wrong.

Commented on

 lazy.assert.string(
      handle,
      `Expected "handle" to be a string, got ${handle}`
    );

pprint at https://searchfox.org/mozilla-central/rev/9962f512b52f5faba6b5a769b9e89a2a72fb8dbf/remote/shared/Format.jsm#42

Comment 1

[sayuree]

Do we need to replace all instances of lazy.assert.string() by pprint?
https://searchfox.org/mozilla-central/search?q=+lazy.assert.string%28&path=&case=false&regexp=false

Comment 2

[Julian Descottes [:jdescottes]]

I think we can focus on the remote/webdriver-bidi folder for now, however we probably need to apply this for all asserts, not only assert.string: https://searchfox.org/mozilla-central/search?q=+lazy.assert&path=remote%2Fwebdriver-bidi&case=false&regexp=false

You can see a usage example at https://searchfox.org/mozilla-central/rev/19f4fa1c9253a783b0ec2664f2bd91ce963aeec0/remote/marionette/action.sys.mjs#1067-1068

I will be away next week, so I am moving the need-info to Henrik, he might have more information.

Comment 3

[sayuree]

Attachment: Bug 1788437 - Add pprint for messages r=jdescottes,whimboo

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I see a patch attached so I'll have a look at the review soon. Thanks sayuree!

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I had a quick look at the patch and as I can see also by the link Julian has given above the changes only cover the remote/webdriver-bidi folder. But we should as best include every folder under remote/ that uses lazy.assert.

By checking various modules I also found the following case which doesn't actually use a template string and as such would not print the real values but just eg. ${parts[0]}.

Sabina, could you have a look and extend the patches? Maybe have different revisions for:

1. shared, modules, server
2. webdriver-bidi
3. marionette
4. cdp

But i'm also fine with you combine it all in a single patch. Thanks a lot.

Comment 6

[sayuree]

Attachment: Bug 1788437 - Add pprint for shared r=whimboo

Depends on D158450

Comment 7

[sayuree]

Attachment: Bug 1788437 - Add pprint for marionette r=whimboo

Depends on D158843

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Sabina, would you have the time to update the patches based on the review comments?

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:whimboo, since the bug doesn't have a severity set, could you please set the severity or close the bug?

For more information, please visit auto_nag documentation.

Comment 11

[gravyant]

Hi, I'd like to take this bug on.

Is my understanding correct that every lazy.assert invocation in remote/ should use pprint? So the actual function being invoked (e.g. lazy.assert.positiveInteger) will stay the same, but the message argument will be refactored to use pprint.

Example:

lazy.assert.positiveInteger(id, `${id}: unsigned integer value expected`) would become

lazy.assert.positiveInteger(id, lazy.pprint`${id}: unsigned integer value expected`)

And then you would also like the error messages to follow a "Expected ... , got ..." format.

Example:

lazy.assert.positiveInteger(id, lazy.pprint`${id}: unsigned integer value expected`) would become

lazy.assert.positiveInteger(id, lazy.pprint`Expected "id" to be an unsigned integer value, got ${id}`)

If so, I'd definitely be willing to take this one. It would be quite a lot of changes for one bug though. Would it be better perhaps to split it up?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to gravyant from comment #11)

Hi, I'd like to take this bug on.

This is great to hear. Thank you Gravyant!

Is my understanding correct that every lazy.assert invocation in remote/ should use pprint? So the actual function being invoked (e.g. lazy.assert.positiveInteger) will stay the same, but the message argument will be refactored to use pprint.

Example:

lazy.assert.positiveInteger(id, `${id}: unsigned integer value expected`) would become

lazy.assert.positiveInteger(id, lazy.pprint`${id}: unsigned integer value expected`)

Yes, that is how it should look like. The helpful details we will get by making this change is that it doesn't only print the value of id but also its type. That way it will be easier to see the difference eg for 4 as a string or a number. But as you asked below please use the other format.

And then you would also like the error messages to follow a "Expected ... , got ..." format.

Example:

lazy.assert.positiveInteger(id, lazy.pprint`${id}: unsigned integer value expected`) would become

lazy.assert.positiveInteger(id, lazy.pprint`Expected "id" to be an unsigned integer value, got ${id}`)

If so, I'd definitely be willing to take this one. It would be quite a lot of changes for one bug though. Would it be better perhaps to split it up?

Yes, that's exactly the right format in how it should look like.

Given that there a lot of changes maybe you can pick-up the work as already attached to the bug. I would propose with D158858. You can try to apply it locally and then only address the remaining work based on the review. Once done we could land it, and you can continue with the next revision on the stack.

If you have further questions please let me know. Thanks again in advance!

Comment 13

[gravyant]

Hey :whimboo, thanks for clarifying. I'd be happy to start working on this!

I might be mistaken, but D158858 references some files that don't exist anymore. For example, remote/marionette/action.js and remote/marionette/element.js. How would you recommend I proceed?

Comment 14

[Alexandra Borovova [:Sasha]]

(In reply to gravyant from comment #13)

Hey :whimboo, thanks for clarifying. I'd be happy to start working on this!

I might be mistaken, but D158858 references some files that don't exist anymore. For example, remote/marionette/action.js and remote/marionette/element.js. How would you recommend I proceed?

Hey! :whimboo is off today, but maybe I can help you :) remote/marionette/action.js was moved and renamed, and it's now remote/shared/webdriver/Actions.sys.mjs. remote/marionette/element.js was split into two modules: remote/marionette/web-reference.sys.mjs and remote/shared/DOM.sys.mjs. So I think you can try to apply the changes manually to these modules.

Will keep needinfo request, in case Henrik wants to add something.

Comment 15

[gravyant]

I won't be able to work on this bug anymore for now, in case anyone wants to pick it up.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to gravyant from comment #15)

I won't be able to work on this bug anymore for now, in case anyone wants to pick it up.

That's sad to hear. Are you interested in something else or won't you have the time for contribution anymore? Maybe you could let us know. Thanks!

Comment 17

[gravyant]

I'll definitely get back to contributing in a few weeks, but I'm at a busy point right now. Thanks for the help!

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thank you for your feedback and no worries! If you want to pick-up this bug again when you have the time just let us know.

Comment 19

[Balarama Raju]

Hi, I'd like to take this bug on.

After reviewing the comments and proposal by gravyant, I believe I have a clear understanding of the necessary changes to address this bug. Thank you for the detailed explanation!

To summarize, the main task involves refactoring every lazy.assert invocation within the remote/ directory to incorporate the use of pprint for the message argument. This means that while the function calls themselves will remain unchanged (e.g., lazy.assert.positiveInteger), the message strings will be transformed using pprint.

Looking forward to your feedback and guidance on how to proceed!

Comment 20

[Julian Descottes [:jdescottes]]

(In reply to Balarama Raju from comment #19)

Hi, I'd like to take this bug on.

After reviewing the comments and proposal by gravyant, I believe I have a clear understanding of the necessary changes to address this bug. Thank you for the detailed explanation!

To summarize, the main task involves refactoring every lazy.assert invocation within the remote/ directory to incorporate the use of pprint for the message argument. This means that while the function calls themselves will remain unchanged (e.g., lazy.assert.positiveInteger), the message strings will be transformed using pprint.

Looking forward to your feedback and guidance on how to proceed!

Hi there! Yes you understood correctly, we want to use pprint for the messages, without changing the actual calls. And the error messages need to be adjusted as well to be more consistent across the codebase. As mentioned by :whimboo on a previous comment, you could probably get started by re-applying https://phabricator.services.mozilla.com/D158858 and addressing the comments already present in the review.

You can take a look at https://firefox-source-docs.mozilla.org/setup/index.html to help you with setting up the tools, and feel free to ask questions here (use the Request information from... feature below the comment box) or on our chat at https://chat.mozilla.org/#/room/#webdriver:mozilla.org

Comment 21

[Balarama Raju]

(In reply to Julian Descottes [:jdescottes] from comment #20)

Hi there! Yes you understood correctly, we want to use pprint for the messages, without changing the actual calls. And the error messages need to be adjusted as well to be more consistent across the codebase. As mentioned by :whimboo on a previous comment, you could probably get started by re-applying https://phabricator.services.mozilla.com/D158858 and addressing the comments already present in the review.
Thanks for confirming. I'd be happy to get started on this.
You can take a look at https://firefox-source-docs.mozilla.org/setup/index.html to help you with setting up the tools, and feel free to ask questions here (use the Request information from... feature below the comment box) or on our chat at https://chat.mozilla.org/#/room/#webdriver:mozilla.org
Sure, thanks for the help.

Comment 22

[Balarama Raju]

Hi Julian, I am done with making code changes. Could you let me if there are any tests that needs to be run before committing the code.

Comment 23

[Balarama Raju]

Attachment: Bug 1788437 - Use pprint for assertion messages in remote dir. r=whimboo,jdescottes

Comment 24

[Julian Descottes [:jdescottes]]

(review ongoing on phabricator)

Comment 25

[Balarama Raju]

Attachment: Bug 1788437 - Use pprint for assertion messages in remote/webdriver--bidi. r=whimboo,jdescottes

Comment 26

[Balarama Raju]

Attachment: Bug 1788437 - Use pprint for assertion messages in remote/shared. r=whimboo,jdescottes

Comment 27

[Balarama Raju]

Attachment: Bug 1788437 - Use pprint for assertion messages in remote/marionette. r=whimboo,jdescottes

Comment 28

[Julian Descottes [:jdescottes]]

Hi there!

Checking back with you to see if you had time to look at the review comments on Phabricator?
Do you need any help to apply them?

Let us know!

Comment 29

[Balarama Raju]

Hi, thanks for checking. I was caught up with some urgent work. I am back now and will address the review comments.

Comment 30

[Julian Descottes [:jdescottes]]

Great, thanks for the update!

Comment 31

[Julian Descottes [:jdescottes]]

Hi Henrik, while discussing on the reviews for this patch, I started wondering why we're not just using pprint once in InvalidArgumentError instead of updating all call sites for assert methods (and I think we should do the same for direct calls to InvalidArgumentError).

If we always want to pprint argument errors, doing it in the Error class could work and be much simpler?

Comment 32

[Julian Descottes [:jdescottes]]

We discussed this in the revision. It's actually not feasible to use pprint after the error message has been built, so we still need to call it immediately when building the string.

Comment 33

[Julian Descottes [:jdescottes]]

Hi! Gentle ping, let me know if you need help with addressing the last comments on the reviews, I think we should be close to landing those patches.

Comment 34

[Julian Descottes [:jdescottes]]

Quick heads up: I plan to land your current patches and just add another changeset implementing my comments quite soon. If you intended to pick them back up soon, let us know. (otherwise the patches will become harder and harder to rebase)

Comment 35

[Julian Descottes [:jdescottes]]

Attachment: Bug 1788437 - [remote] Update pprint call sites to avoid pretty printing known strings

Depends on D212469

Comment 36

[Julian Descottes [:jdescottes]]

Attachment: Bug 1788437 - [wdspec] Update script invalid tests to properly check invalid optional arguments

Depends on D216811

Comment 37

[Phabricator Automation]

Comment on attachment 9413368 [details]
Bug 1788437 - [wdspec] Update script invalid tests to properly check invalid optional arguments

Revision D216865 was moved to bug 1908621. Setting attachment 9413368 [details] to obsolete.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dabd8e852340
Use pprint for assertion messages in remote/webdriver--bidi. r=jdescottes,webdriver-reviewers
https://hg.mozilla.org/integration/autoland/rev/79b73529942d
Use pprint for assertion messages in remote/shared. r=jdescottes,webdriver-reviewers
https://hg.mozilla.org/integration/autoland/rev/d7f49241dc26
Use pprint for assertion messages in remote/marionette. r=jdescottes,webdriver-reviewers
https://hg.mozilla.org/integration/autoland/rev/5aac416897b3
[remote] Update pprint call sites to avoid pretty printing known strings r=webdriver-reviewers,whimboo

Comment 39

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/dabd8e852340
https://hg.mozilla.org/mozilla-central/rev/79b73529942d
https://hg.mozilla.org/mozilla-central/rev/d7f49241dc26
https://hg.mozilla.org/mozilla-central/rev/5aac416897b3

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks a lot Balarama for your work on this bug and it's indeed great to see the consistency now!