Closed Bug 1866273 Opened 3 months ago Closed 3 months ago

Use thiserror crate for webdriver

Categories

(Testing :: geckodriver, enhancement)

Default
enhancement

Tracking

(firefox122 fixed)

RESOLVED FIXED
122 Branch
Tracking Status
firefox122 --- fixed

People

(Reporter: whimboo, Assigned: jameshendry05, Mentored)

References

Details

(Whiteboard: [lang=rust])

Attachments

(1 file)

With bug 1739307 we got the thiserror crate to be used for our mozbase-rust crates. Now we can also update our webdriver code to make use of it as well.

Code that needs to be updated:

https://searchfox.org/mozilla-central/search?q=impl+Error&path=testing%2Fwebdriver&case=false&regexp=false

Component: Mozbase Rust → geckodriver

Would love to work on more of this. I already had my eye on what to do next and seems that https://bugzilla.mozilla.org/show_bug.cgi?id=1682239 is a partial duplicate of this that you made a while ago. Happy to work on both/either though, just let me know which bug to target first.

Blocks: 1682239

Ah good point! I completely missed that other bug. I've updated it now so that it will cover the anyhow create for geckodriver, while for the webdriver crate we should still use thiserror.

Glad to see that you are interested James!

Assignee: nobody → jameshendry05
Status: NEW → ASSIGNED

From what I can see the only thing that implments the Error trait in webdriver is WebDriverError, so this is the only thing I have converted.


Additionally something I wanted to flag up:
ttps://searchfox.org/mozilla-central/source/testing/webdriver/src/error.rs#209

UnknownCommand | UnknownError => "unknown error",

But:
https://searchfox.org/mozilla-central/source/testing/webdriver/src/error.rs#285

"unknown command" => UnknownCommand,
"unknown error" => UnknownError,

This seems odd to me - it means that converting an "unknown command" to the enum, then re-serialising it would yield "unknown error". This may be intentional though - in which case maybe a reference / comment may be warranted.

(In reply to jameshendry05 from comment #4)

From what I can see the only thing that implments the Error trait in webdriver is WebDriverError, so this is the only thing I have converted.

Great, I will have a look soon. And yes, there shouldn't be much more to do.

Additionally something I wanted to flag up:
https://searchfox.org/mozilla-central/source/testing/webdriver/src/error.rs#209

UnknownCommand | UnknownError => "unknown error",

Oh, good find! It looks like a bug to me that we should get fixed. From what I can see it should actually be or'ed with UnknownPath given that this will also lead to the unknown command error. Would you mind filing a new bug for that in the Testing :: geckodriver component?

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5448f8eddf29
Use thiserror crate for webdriver r=webdriver-reviewers,jgraham
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 122 Branch

James, again thanks a lot for your contribution! In case you have still interest on bug 1682239 I would appreciate that, because then we have a better error handling across all of our rust crates related to geckodriver.

Blocks: 1824713
You need to log in before you can comment on or make changes to this bug.