1866273 - Use thiserror crate for webdriver

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement)

Description

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

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

Comment 1

[jameshendry05]

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.

Comment 2

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

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!

Comment 3

[jameshendry05]

Attachment: Bug 1866273: Use thiserror crate for webdriver r=whimboo

Comment 4

[jameshendry05]

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.

Comment 5

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

(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?

Comment 6

[jameshendry05]

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=1866776

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5448f8eddf29
Use thiserror crate for webdriver r=webdriver-reviewers,jgraham

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5448f8eddf29

Comment 9

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

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.