Use pprint for assertion messages
Categories
(Remote Protocol :: Agent, task, P3)
Tracking
(firefox130 fixed)
Tracking | Status | |
---|---|---|
firefox130 | --- | fixed |
People
(Reporter: jdescottes, Assigned: sbalaramaraju2000, Mentored)
Details
(Whiteboard: [lang=js][webdriver:m12][webdriver:external])
Attachments
(4 files, 5 obsolete files)
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}`
);
Reporter | ||
Updated•2 years ago
|
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®exp=false
Reporter | ||
Comment 2•2 years ago
|
||
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®exp=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.
Updated•2 years ago
|
Comment 4•2 years ago
|
||
I see a patch attached so I'll have a look at the review soon. Thanks sayuree!
Comment 5•2 years ago
|
||
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:
- shared, modules, server
- webdriver-bidi
- marionette
- cdp
But i'm also fine with you combine it all in a single patch. Thanks a lot.
Updated•2 years ago
|
Comment 8•2 years ago
|
||
Hi Sabina, would you have the time to update the patches based on the review comments?
Updated•2 years ago
|
Updated•2 years ago
|
Comment 9•1 year ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Comment 10•1 year ago
|
||
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.
Updated•9 months ago
|
Comment 11•5 months ago
|
||
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•5 months ago
|
||
(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 inremote/
should usepprint
? So the actual function being invoked (e.g.lazy.assert.positiveInteger
) will stay the same, but the message argument will be refactored to usepprint
.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•5 months ago
|
||
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•5 months ago
|
||
(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
andremote/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•4 months ago
|
||
I won't be able to work on this bug anymore for now, in case anyone wants to pick it up.
Comment 16•4 months ago
|
||
(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•4 months ago
|
||
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•4 months ago
|
||
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.
Assignee | ||
Comment 19•4 months ago
|
||
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!
Reporter | ||
Comment 20•4 months ago
|
||
(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
Assignee | ||
Comment 21•4 months ago
|
||
(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.
Assignee | ||
Comment 22•4 months ago
|
||
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.
Assignee | ||
Comment 23•4 months ago
|
||
Updated•4 months ago
|
Assignee | ||
Comment 25•3 months ago
|
||
Updated•3 months ago
|
Assignee | ||
Comment 26•3 months ago
|
||
Assignee | ||
Comment 27•3 months ago
|
||
Updated•3 months ago
|
Updated•3 months ago
|
Updated•3 months ago
|
Updated•3 months ago
|
Reporter | ||
Comment 28•3 months ago
|
||
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!
Assignee | ||
Comment 29•3 months ago
|
||
Hi, thanks for checking. I was caught up with some urgent work. I am back now and will address the review comments.
Reporter | ||
Comment 31•3 months ago
|
||
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?
Reporter | ||
Comment 32•3 months ago
|
||
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.
Updated•3 months ago
|
Reporter | ||
Comment 33•2 months ago
|
||
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.
Reporter | ||
Updated•2 months ago
|
Reporter | ||
Comment 34•2 months ago
|
||
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)
Updated•2 months ago
|
Comment 37•2 months ago
|
||
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•2 months ago
|
||
Pushed by jdescottes@mozilla.com: 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•2 months ago
|
||
bugherder |
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•2 months ago
|
||
Thanks a lot Balarama for your work on this bug and it's indeed great to see the consistency now!
Description
•