Closed
Bug 1396821
Opened 7 years ago
Closed 6 years ago
Use serde in place of rustc_serialize
Categories
(Testing :: geckodriver, enhancement, P1)
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: ato, Assigned: whimboo)
References
(Depends on 1 open bug, Blocks 2 open bugs)
Details
Attachments
(6 files, 8 obsolete files)
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
jgraham in https://github.com/mozilla/webdriver-rust/issues/94: > In the long term it would be a good idea to use serde instead > of rustc_serialize. This will allow us to derive a lot of the > implementation from the struct definitions, and should remove a > great deal of boilerplate code dealign with type conversions and > errors. The serde branch contains a (bitrotted) first attempt at > this; it seemed mostly straightforward except for the actions API > where the internal representation uses a different struct based > on the action field of the message; it wasn't entirely clear to > me how to map that using any of the provided attributes so it's > probably necessary to write some custom derive/serialize logic for > those parts (or deserialize to a struct that matches the protocol > and then convert that into the form that matches the data model). > > This transition should also be accompanied by some improved tests > for the low level parts of the protocol to ensure that we don't > regress existing behaviour.
Reporter | ||
Comment 1•7 years ago
|
||
dtolnay:
> Thanks @jgraham! I didn't find the place in the code dealing with
> action but I think Serde's internally tagged enum may be what you
> need.
>
>> extern crate serde;
>>
>> #[macro_use] extern crate serde_json;
>> #[macro_use] extern crate serde_derive;
>>
>> use serde::Deserialize;
>>
>> #[derive(Deserialize, Debug)]
>> #[serde(tag = "action", rename_all = "lowercase")]
>> enum Action {
>> Smile(Smile),
>> Frown { muscles: u16 },
>> }
>>
>> #[derive(Deserialize, Debug)]
>> struct Smile {
>> seconds: u64,
>> }
>>
>> fn main() {
>> let smile = json!({ "action": "smile", "seconds": 2 });
>> println!("{:?}", Action::deserialize(smile).unwrap());
>>
>> let frown = json!({ "action": "frown", "muscles": 11 });
>> println!("{:?}", Action::deserialize(frown).unwrap());
>> }
Reporter | ||
Comment 3•7 years ago
|
||
jgraham:
> So it seems that we are also missing
> https://github.com/serde-rs/serde/issues/119
>
> The problem is that we have something that looks like:
>
>> {"id": null,
>> "type": "pointer",
>> "parameters": {"pointerType": "mouse"},
>> "actions": [β¦]}
>
> or
>
>> {"id": null,
>> "type": "key",
>> "actions": [β¦]}
>
> So parameters can only exist for the pointer-type actions. So this
> is modelled as a set of nested enums:
>
>> enum ActionsSequence {
>> id: Option<String>,
>> actions: ActionsType
>> }
>>
>> enum ActionsType {
>> Key(Vec<KeyActions>),
>> Pointer {parameters: PointerParameters, Vec<PointerActions>}
>> }
>
> So obviously what's needed is to be able to flatten the
> ActionsType enum into the ActionsSequence, but that currently
> seems to be difficult.
Reporter | ||
Comment 4•7 years ago
|
||
dtolnay:
> Here is how I would handle it. (Or contribute a flatten
> implementation!)
>
>> impl<'de> Deserialize<'de> for ActionsSequence {
>> fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
>> where D: Deserializer<'de>
>> {
>> #[derive(Deserialize)]
>> #[serde(tag = "type", rename_all = "lowercase")]
>> enum Helper {
>> Key {
>> id: Option<String>,
>> actions: Vec<KeyActions>,
>> },
>> Pointer {
>> id: Option<String>,
>> parameters: PointerParameters,
>> actions: Vec<PointerActions>,
>> },
>> }
>>
>> match Helper::deserialize(deserializer)? {
>> Helper::Key { id, actions } => {
>> Ok(ActionsSequence {
>> id: id,
>> actions: ActionsType::Key(actions),
>> })
>> }
>> Helper::Pointer { id, parameters, actions } => {
>> Ok(ActionsSequence {
>> id: id,
>> actions: ActionsType::Pointer(parameters, actions),
>> })
>> }
>> }
>> }
>> }
Reporter | ||
Comment 5•7 years ago
|
||
whimboo:
> @jgraham what do you think about the proposal from @dtolnay?
Reporter | ||
Comment 6•7 years ago
|
||
jonhoo:
> It'd be great to see some progress on this so we can drop the
> dependency on the deprecated rustc-serialize
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 7•7 years ago
|
||
As we discussed today our proposal to get this done would be the following: 1) Extract the differences from the serde branch on the webdriver-rust repository 2) Modify the paths so that the patch could be applied to mozilla-central 3) Fix bit-rotted content, and update those parts which easily could use Serde 4) Revert those parts which remove rustc_serialize, so we can defer the conversion for Actions to a follow-up bug 5) Check which webdriver spec tests currently exist [1]. If commands are not covered try to write some new, or at least create unit tests in webdriver-rust itself I will have a look at the first two items now, and will upload the modified patch shortly. [1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/webdriver
Assignee | ||
Comment 8•7 years ago
|
||
Instead use serde. This is the simplest possible conversion using the serde Value type everywhere. The intent is to use the atuomatically derived deserializers in the future.
Assignee: nobody → hskupin
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
As attached you can find the two patches exported directly from the github repository. To apply them do the following: $ cd testing/webdriver $ patch -p0 <Remove_rustc_serialize_dependency.patch (lots of hunks will fail) $ patch -p0 <Use_serde_[de]serialize (not tested yet) This will be actually a bit of work to update the patches to apply to recent mozilla-central. It's more as we originally thought.
Assignee: hskupin → nobody
Assignee | ||
Comment 11•7 years ago
|
||
As it looks like rebasing directly via git based on the code from the Github repository seems to work way better. I'm fiddling through the merge conflicts now to get the patch applying correctly on recent code.
Assignee | ||
Comment 12•7 years ago
|
||
By using git it's still a mess given all the recent changes, but I'm making my way through. While doing it I decided to split up parts into individual commits for easier reviewing. Especially the move of all actions related code into its own file, and the various modifications since May makes it hard to sync without missing parts.
Assignee | ||
Comment 13•7 years ago
|
||
And to note, rebasing on the github repo is against the `old` branch, which is a bit outdated. So once the commits are getting imported into mozilla-central another round of rebasing would be necessary.
Assignee | ||
Comment 14•7 years ago
|
||
I was able to completely rebase the serde branch on github, and got everything compiled. I will export the individual patches to mozilla-central now. As it looks like there is not that much left to do, I will pick this bug up.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Assignee | ||
Comment 15•7 years ago
|
||
The second rebase from changeset fb3c4b567fe2 (import of webdriver) to tip of mozilla-central brought in a couple of changes which needed extra care in updating. I have a patch now which at least applies cleanly on top of mozilla-central, and can be built successfully with cargo. So far I haven't made any other code changes beside those James already did in his version. After uploading it I would check how try results look like, so that we can see where failures exists.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8910472 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8910474 -
Attachment is obsolete: true
Assignee | ||
Comment 19•7 years ago
|
||
Only updating webdriver doesn't work here. Given that the crate is in mozilla-central now, we also have to upgrade geckodriver for using serde instead of rustc_serialize at the same step. I will continue tomorrow.
Assignee | ||
Comment 20•7 years ago
|
||
Updating geckodriver with the 2-step process as done for webdriver is a mess. I don't actually want to write intermediate code which gets removed again with the next commit. As such we agreed on to combine both commits into a single one.
The update which I will push now only contains this merge, and updates to the tests. I will write more tests on Monday for Parameters (input) and Responses (output) so that we can be sure to not have regressed something.
What I'm also not happy with are all the methods of the following form:
> impl<'a> From<&'a WindowRectParameters> for Value {
I assume those are needed because we basically assume always a `Value` type, but do not have an enum of what we really expect. Once we have that we should be able to get rid of most of those methods too.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8910912 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•7 years ago
|
||
The last update adds new tests for actions. It's not complete given that I have issues with serialization and deserialization of PointerOrigin and ActionSequence, which means that serde fails by using the expected JSON representation of an action sequence. I will further check later this week. In case someone has an idea please let me know.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•7 years ago
|
||
Given that the patch hasn't been rebased for a while, I did it now to not let it bitrot again that much. There were actually a couple of conflicts, which are resolved now. Not sure yet when I will be able to continue.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8912283 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•7 years ago
|
||
With the last update we can now also successfully serialize / deserialize all the actions sequences! Note that I still have to get a lot of new tests added for all the other modules in the webdriver crate. Once that is done we also have to start updating geckodriver itself. Something I don't like with the huge amount of tests (half of the actions module) is that those are part of the actual module. I really would like to see them separated which will improve compilation time (yes I don't want to wait each time I update a test until the module has been compiled), and makes the code cleaner.
Assignee | ||
Comment 38•7 years ago
|
||
James, you mentioned that you also did the changes for geckodriver, but sadly those got lost on the remote end while cleaning up the repository. Would you mind to check if you still have that branch locally in case the github repo is still cloned on your disk? Otherwise I would have to start from fresh. Thanks.
Flags: needinfo?(james)
Comment 39•7 years ago
|
||
No, the point was I lost the changes entirely. I don't know how that happened and it's really annoying, but it is what it is.
Flags: needinfo?(james)
Assignee | ||
Comment 40•6 years ago
|
||
mozreview-review |
Comment on attachment 8910911 [details] Bug 1396821 - Switch from rustc_serialize to serde_json. https://reviewboard.mozilla.org/r/182200/#review257006 I didn't find it useful to keep the two independent changesets because it takes me always a bit of time to fix rebase issues. As such I combined both into a single commit.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8910911 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8910910 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•6 years ago
|
||
The last update of the patch is done to be sure nothing gets lost locally. I did a lot of work by removing the custom implementations which were necessary for rustc_serialize. Also lots and lots of new tests have been added. I hope that I will be able to continue soon.
Assignee | ||
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8988570 -
Attachment is obsolete: true
Assignee | ||
Comment 46•6 years ago
|
||
The last update finalizes the serde conversion for the webdriver crate. As of now I don't know if I have removed too much, which might be necessary later by geckodriver. Before I can say something here, I will have to do the conversion for the geckodriver crate too. Overall 2000 lines have been removed, but also 2000 lines been added, whereby all of those are unit tests and which are about 143 of them.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 49•6 years ago
|
||
The recent update contains the changes for geckodriver from rustc_serialize to serde, which is not complete yet! I'm still trying to get it to compile, and once done the refactoring will start.
Assignee | ||
Comment 50•6 years ago
|
||
With the latest changes as done yesterday I got both webdriver and geckodriver to compile. I also fixed the `From` trait to correctly convert from Serde Error to WebDriverError. Error messages look fine now in case of JSON data contains invalid content. When I tried to run geckodriver via wdspec tests, or Selenium I noticed that the `New Session` command fails. This was all happening due to problems in deserialization. I was able to fix most of them, but two problems with the `SpecNewSessionParameters` struct are remaining which I want to see fixed before uploading the next version of the patch.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Priority: P3 → P1
Assignee | ||
Comment 54•6 years ago
|
||
The latest patch has a lot of changes and results in nearly all wdspec tests passing. Only two WebDriver commands are missing to fix: * Anything related to Actions. As we know the JSON body is complex and there is something wrong at the top-level structs. * New Session is complicated due to the legacy and spec compliant capabilities matching. While basic new session tests are working, there is work left to do for merging. Problem is that with Serde's default deserialization we can only accept spec capabilities OR legacy ones. Beside those commands all :moz Firefox specific commands need an update. Note that we don't have any webdriver tests for those, and I might have to get at least the basics implemented under `testing/web-platform/mozilla/tests/webdriver/tests`.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 57•6 years ago
|
||
With the last update all of the Action wdspec tests are working now. By tomorrow I will concentrate on the new session tests, and if time permits on the moz: prefixed ones.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8998754 -
Attachment is obsolete: true
Assignee | ||
Comment 61•6 years ago
|
||
With the current state of code we should pass all wdspec tests now. At least locally no existing test fails, same for all the newly added unit tests for geckodriver and webdriver. As such I pushed a try build via mozreview. Work still do do: * Make all moz: prefixed commands to use Serde * Maybe instead of adding Rust unit tests, move all those tests to the wdspec test suite, so that all drivers can benefit from. Maybe this should only be done for the invalid cases, so that we can ensure that Serde deserializes and serializes valid data correctly.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 65•6 years ago
|
||
The triggered try build has failures for deserializing/serializing the FrameId struct. A fix for this will be included in the next version of the patch. Meanwhile I also got serialization/deserialization working for: * GetContext, * SetContext(GeckoContextParameters), * InstallAddon(AddonInstallParameters), * UninstallAddon(AddonUninstallParameters) Those commands are remaining now: * XblAnonymousChildren(WebElement), * XblAnonymousByAttribute(WebElement, AttributeParameters),
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 69•6 years ago
|
||
FYI Wdspec jobs are green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4994a99ee2e2b34efefac7c9dcc84a8f9579588b
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #9000187 -
Flags: review?(ato)
Attachment #8985061 -
Flags: review?(james)
Attachment #8985061 -
Flags: review?(ato)
Attachment #9002509 -
Flags: review?(ato)
Attachment #8997382 -
Flags: review?(james)
Attachment #8997382 -
Flags: review?(ato)
Attachment #9002510 -
Flags: review?(ato)
Attachment #9001256 -
Flags: review?(ato)
Assignee | ||
Comment 76•6 years ago
|
||
To test the Gecko extension commands I created local Selenium tests, and I can verify that those all are still working. I will attach those to bug 1483559, which I want to do as a follow-up of this bug.
Assignee | ||
Comment 77•6 years ago
|
||
Note that by using Serde we have an increase of the binary size, eg. for Linux64 opt from 28.610 KB to 31.160 KB as build by our CI. For official releases the difference should not be that huge, whereby the binaries are also way smaller.
Reporter | ||
Comment 78•6 years ago
|
||
So admit I havenβt read all the comments on this bug. Let me know if they provide essential information to reviewing the patches otherwise I will make a head start on that. Just a tiny comment about the binary size, I think youβre right in expecting it will increase somewhat (mostly due to all the derives I imagine) but I would expect the difference for optimised builds to be smaller since they have more intensive code path elimination and so on. Apart from that, see https://bugzilla.mozilla.org/show_bug.cgi?id=1392313 for some of my investigations into reducing the binary size.
Reporter | ||
Comment 79•6 years ago
|
||
mozreview-review |
Comment on attachment 9001256 [details] Bug 1396821 - Update vendored Rust crates. https://reviewboard.mozilla.org/r/261696/#review269624
Attachment #9001256 -
Flags: review?(ato) → review+
Reporter | ||
Comment 80•6 years ago
|
||
mozreview-review |
Comment on attachment 9000187 [details] Bug 1396821 - [wdspec] Mark all tests in new_session/merge.py to pass. https://reviewboard.mozilla.org/r/261688/#review269626
Attachment #9000187 -
Flags: review?(ato) → review+
Assignee | ||
Comment 81•6 years ago
|
||
All the comments above only gave status updates. So nothing you will have to actually read through. The only thing you should be aware of are still used calls to `unwrap()` which are used in one or two places, and I'm not sure how to get rid of those. Mainly because they are inside a `map` lambda.
Reporter | ||
Comment 82•6 years ago
|
||
mozreview-review |
Comment on attachment 9002510 [details] Bug 1396821 - [geckodriver] Apply rustfmt other geckodriver modules. https://reviewboard.mozilla.org/r/261706/#review269628
Attachment #9002510 -
Flags: review?(ato) → review+
Reporter | ||
Comment 83•6 years ago
|
||
mozreview-review |
Comment on attachment 9002509 [details] Bug 1396821 - [geckodriver] Apply rustfmt on other webdriver modules. https://reviewboard.mozilla.org/r/261704/#review269630
Attachment #9002509 -
Flags: review?(ato) → review+
Reporter | ||
Comment 84•6 years ago
|
||
mozreview-review |
Comment on attachment 8985061 [details] Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/250782/#review269632 This is obviously such a big and invasive change that it is hard for me as a reviewer to look at every minor detail. Overall Iβm very happy about this change, especially with the inline Rust tests. I understand you will want to turn some of these into WPT tests in the future, which is a great idea. The serialisation of inner values of single-field structs (e.g. CookieResponse, ValueResponse, et al.) is an obvious blemish on this patch, but since they seem to work Iβm not too bothered about this. ::: testing/webdriver/src/response.rs:57 (Diff revision 10) > +#[derive(Clone, Debug, PartialEq)] > +pub struct CookieResponse { > + pub cookie: Cookie, > +} > + > +impl Serialize for CookieResponse { > + fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> > + where > + S: Serializer, > + { > + self.cookie.serialize(serializer) > + } > +} I wonder if it might allow you to skip the internal serialiser if you define the struct this way, without the explicit fields? > #[derive(Clone, Debug, PartialEq, Serialize)] > pub struct CookieResponse(Cookie); ::: testing/webdriver/src/response.rs:87 (Diff revision 10) > - Json::Array(self.window_handles > - .iter() > - .map(|x| Json::String(x.clone())) > - .collect::<Vec<Json>>()) > + /// X axis position of the top-left corner of the element relative > + // to the current browsing contextβs document element in CSS reference > + // pixels. > + pub x: f64, The two last lines appear to have lost a "/". API documentation always starts with three "///" to distinguish it from regular comments. ::: testing/webdriver/src/response.rs:92 (Diff revision 10) > + /// Y axis position of the top-left corner of the element relative > + // to the current browsing contextβs document element in CSS reference > + // pixels. > + pub y: f64, Same problem here. ::: testing/webdriver/src/response.rs:110 (Diff revision 10) > +impl ElementRectResponse { > + pub fn new(x: f64, y: f64, width: f64, height: f64) -> ElementRectResponse { > + ElementRectResponse { > + x: x, > + y: y, > + width: width, > + height: height, > + } > } > } I canβt see this is used anywhere? In any case I would prefer not to use a ::new constructor for structs like these because it is hard to remember the order of x, y, width, and height. Instead it is easier to read ElementRectResponse { x: 42, y: 23, β¦ }. ::: testing/webdriver/src/response.rs:153 (Diff revision 10) > -#[derive(Debug, RustcEncodable)] > +#[derive(Debug, PartialEq)] > pub struct ValueResponse { > - pub value: json::Json > + pub value: Value, > } > > -impl ValueResponse { > - pub fn new(value: json::Json) -> ValueResponse { > - ValueResponse { > - value: value > +impl Serialize for ValueResponse { > + fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> > + where > + S: Serializer, > + { > + self.value.serialize(serializer) > - } > + } > - } > +} I donβt know serde, but why canβt serde infer which inner field is meant to be serialised here? ::: testing/webdriver/src/response.rs:203 (Diff revision 10) > - fn test(resp: WebDriverResponse, expected_str: &str) { > - let data = resp.to_json_string(); > - let actual = Json::from_str(&*data).unwrap(); > - let expected = Json::from_str(expected_str).unwrap(); > - assert_eq!(actual, expected); > + #[test] > + fn test_json_close_window_response() { > + let json = r#"{"value":["1234"]}"#; > + let data = WebDriverResponse::CloseWindow(CloseWindowResponse::new(vec!["1234".into()])); > + > + check_serialize(&json, &data); > } So this can also be an empty array, but I donβt expect we need to test that explicitly? ::: testing/webdriver/src/server.rs:227 (Diff revision 10) > } > - Err(err) => (err.http_status(), err.to_json_string()), > + Err(err) => (err.http_status(), > + serde_json::to_string_pretty(&err).unwrap()), > }; > > - debug!("<- {} {}", status, resp_body); > + debug!("<- {} {:?}", status, resp_body); Perhaps serde_json::to_string_pretty here as well to get what is actually transmitted across the wire? ::: testing/webdriver/src/test.rs:6 (Diff revision 10) > +use regex::Regex; > +use serde; > +use serde_json; > +use std; > + > +pub fn check_serialize_deserialize<T>(json: &str, data: &T) You probably want to wrap this entire module in a #[cfg(test)] block so it doesnβt get shipped with the library. ::: testing/webdriver/src/test.rs:13 (Diff revision 10) > + lazy_static! { > + static ref MIN_REGEX: Regex = Regex::new(r"[\n\t]|\s{4}").unwrap(); > + } Nit: I think you should probably be able to define this at the module level, e.g. outside of each function?
Attachment #8985061 -
Flags: review?(ato) → review+
Reporter | ||
Comment 85•6 years ago
|
||
mozreview-review |
Comment on attachment 8997382 [details] Bug 1396821 - [geckodriver] Switch geckodriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/261172/#review269634
Attachment #8997382 -
Flags: review?(ato) → review+
Reporter | ||
Comment 86•6 years ago
|
||
Let me sum up my position on this changeset: The changes are so big and invasive that it is hard to tell from code review alone if the changes βare soundβ. Because there is so much other work depending on the serde conversion I would propose that we accept the patches mostly as they are. We can file follow-ups with smaller patches to address any stylistic or performance issues associated with some of the explicit serialisers. The second argument I would like to make is that we need to test these changes thoroughly and the best way of doing that is in central over time. This means we will be delaying the next release of geckodriver until we can be sure this isnβt going to cause any severe problems for users. geckodriver is still in a pre-release state so we are making these (potentially breaking) changes at the right point in time.
Assignee | ||
Comment 87•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8985061 [details] Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/250782/#review269632 > I wonder if it might allow you to skip the internal serialiser if > you define the struct this way, without the explicit fields? > > > #[derive(Clone, Debug, PartialEq, Serialize)] > > pub struct CookieResponse(Cookie); Probably. I will have a look. > I donβt know serde, but why canβt serde infer which inner field is > meant to be serialised here? The problem here is related to deserializing the Marionette response. If I remove it we would end-up with a nested `value` key like: `"{\"value\":{\"value\":{\"example\":[\"test\"]}}}"`. I will have a look if that is easy enough. Otherwise we could fix that with the follow-up bug for Marionette serialization/deserialization. > So this can also be an empty array, but I donβt expect we need to > test that explicitly? There shouldn't be a need for that. Given that our data structure is valid, serialization should always pass even for an empty array. Otherwise something in Serde would be horrible broken. > Perhaps serde_json::to_string_pretty here as well to get what is > actually transmitted across the wire? `resp_body` is already the JSON string, so no reason to serialize it again. But `to_string_pretty()` is something James might have added in his initial patch. For the error the line above I find it kinda annoying, and it also changes the behavior. Right now we don't prettify anything. I think we should revert that to just `to_string()` to keep small logs. > You probably want to wrap this entire module in a #[cfg(test)] block > so it doesnβt get shipped with the library. I tried that before but got a failure that the module is unknown. And that all when running `cargo test`. Maybe I missed something, given that it works now. > Nit: I think you should probably be able to define this at the > module level, e.g. outside of each function? Good idea.
Reporter | ||
Comment 88•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8985061 [details] Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/250782/#review269632 > `resp_body` is already the JSON string, so no reason to serialize it again. > > But `to_string_pretty()` is something James might have added in his initial patch. For the error the line above I find it kinda annoying, and it also changes the behavior. Right now we don't prettify anything. I think we should revert that to just `to_string()` to keep small logs. My concern here was actually the change from "{}" to "{:?}".
Assignee | ||
Comment 89•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8985061 [details] Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/250782/#review269632 > My concern here was actually the change from "{}" to "{:?}". Oh, that one I can also remove.
Assignee | ||
Comment 90•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8985061 [details] Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/250782/#review269632 > I tried that before but got a failure that the module is unknown. And that all when running `cargo test`. Maybe I missed something, given that it works now. Well, no I can remember. It was working fine for the tests of the webdriver crate. But keeping it under `cfg(test)` doesn't allow me to use those methods for geckdriver. Any idea how to get those shared? Or would I have to duplicate those?
Reporter | ||
Comment 91•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8985061 [details] Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/250782/#review269632 > Well, no I can remember. It was working fine for the tests of the webdriver crate. But keeping it under `cfg(test)` doesn't allow me to use those methods for geckdriver. Any idea how to get those shared? Or would I have to duplicate those? Oh so you want to re-use them for tests in geckodriver. I would suggest to duplicate them as we donβt want to have them as part of the published webdriver API.
Assignee | ||
Comment 92•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8985061 [details] Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/250782/#review269632 > Probably. I will have a look. I checked that for both the CookieResponse and CookiesResponse and it works. Except that we need a custom implementation for the constructor of both structs because tuple structs have private fields by default. See https://github.com/rust-lang/rust/issues/42944. > Oh so you want to re-use them for tests in geckodriver. I would > suggest to duplicate them as we donβt want to have them as part of > the published webdriver API. I will duplicate the code then. Thanks.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 99•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8985061 [details] Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/250782/#review269632 > I checked that for both the CookieResponse and CookiesResponse and it works. Except that we need a custom implementation for the constructor of both structs because tuple structs have private fields by default. See https://github.com/rust-lang/rust/issues/42944. Well, it's actually possible to use `pub` as usual to make it public. So no custom constructor implementations are necessary.
Comment 100•6 years ago
|
||
mozreview-review |
Comment on attachment 8985061 [details] Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/250782/#review269640 ::: testing/webdriver/src/response.rs:6 (Diff revision 10) > -use rustc_serialize::json::{self, Json, ToJson}; > +use serde_json::Value; > -use std::collections::BTreeMap; > -use time; > > -#[derive(Debug)] > +#[derive(Debug, PartialEq, Serialize)] > +#[serde(untagged, remote = "Self")] Why is the `remote` attribute needed? ::: testing/webdriver/src/server.rs:217 (Diff revision 10) > Ok(data) => { > match data { > - Ok(response) => (StatusCode::Ok, response.to_json_string()), > - Err(err) => (err.http_status(), err.to_json_string()), > + Ok(response) => (StatusCode::Ok, > + serde_json::to_string_pretty(&response).unwrap()), > + Err(err) => (err.http_status(), > + serde_json::to_string_pretty(&err).unwrap()), I wonder if it's worthwhile sending a pretty-formatted string here vs using the default (presumably smaller) representation.
Attachment #8985061 -
Flags: review?(james)
Assignee | ||
Comment 101•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8985061 [details] Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/250782/#review269640 > Why is the `remote` attribute needed? That is what dtolney proposed to me. So I didn't actually question it. Do you think it is not necessary here? > I wonder if it's worthwhile sending a pretty-formatted string here vs using the default (presumably smaller) representation. That got already reverted with my last update as pushed some minutes ago.
Assignee | ||
Comment 102•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8985061 [details] Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde. https://reviewboard.mozilla.org/r/250782/#review269640 > That is what dtolney proposed to me. So I didn't actually question it. Do you think it is not necessary here? Actually this should be necessary because the serialization is forwarded to the Wrapper as defined in the Serialize method for WebDriverResponse.
Assignee | ||
Updated•6 years ago
|
Attachment #8997382 -
Flags: review?(james)
Comment 103•6 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ef3912feb2c [wdspec] Mark all tests in new_session/merge.py to pass. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/b61ce753f01e [geckodriver] Switch webdriver crate from rustc_serialize to serde. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/7c97853a85b9 [geckodriver] Apply rustfmt on other webdriver modules. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/5c0ddd45f926 [geckodriver] Switch geckodriver crate from rustc_serialize to serde. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/c5895db52483 [geckodriver] Apply rustfmt other geckodriver modules. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/2fa2975f97e3 Update vendored Rust crates. r=ato
Comment 104•6 years ago
|
||
Backed out for linux tier2 build bustages Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2fa2975f97e3e81f501aa982ab224ac470683f51 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=195092647&repo=mozilla-inbound&lineNumber=43221 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a7212c51a48253bfa9551ecc9318a9536c62d4
Flags: needinfo?(hskupin)
Comment 105•6 years ago
|
||
There is also this failure: https://treeherder.mozilla.org/logviewer.html#?job_id=195101936&repo=mozilla-inbound&lineNumber=2111 [task 2018-08-21T17:06:42.619Z] 17:06:42 INFO - STDOUT: value = None [task 2018-08-21T17:06:42.619Z] 17:06:42 INFO - STDOUT: tests/web-platform/tests/webdriver/tests/support/asserts.py [task 2018-08-21T17:06:42.620Z] 17:06:42 INFO - STDOUT: :62: AssertionError [task 2018-08-21T17:06:42.621Z] 17:06:42 INFO - STDOUT: =============================== warnings summary =============================== [task 2018-08-21T17:06:42.621Z] 17:06:42 INFO - STDOUT: <undetermined location> [task 2018-08-21T17:06:42.622Z] 17:06:42 INFO - STDOUT: Module already imported so cannot be rewritten: mozlog [task 2018-08-21T17:06:42.622Z] 17:06:42 INFO - STDOUT: -- Docs: http://doc.pytest.org/en/latest/warnings.html [task 2018-08-21T17:06:42.622Z] 17:06:42 INFO - STDOUT: ================ 1 failed, 5 passed, 1 warnings in 3.23 seconds ================ [task 2018-08-21T17:06:42.639Z] 17:06:42 INFO - [task 2018-08-21T17:06:42.639Z] 17:06:42 INFO - TEST-PASS | /webdriver/tests/actions/bounds.py | test_pause_positive_integer[none] [task 2018-08-21T17:06:42.639Z] 17:06:42 INFO - TEST-PASS | /webdriver/tests/actions/bounds.py | test_pause_positive_integer[key] [task 2018-08-21T17:06:42.639Z] 17:06:42 INFO - TEST-UNEXPECTED-FAIL | /webdriver/tests/actions/bounds.py | test_pause_positive_integer[pointer] - AssertionError: invalid argument (400): missing field `parameters` at line 1 column 95 [task 2018-08-21T17:06:42.640Z] 17:06:42 INFO - session = <Session d2d57418-ae20-44fe-b61e-0358c9de07b2> [task 2018-08-21T17:06:42.640Z] 17:06:42 INFO - action_type = 'pointer' [task 2018-08-21T17:06:42.640Z] 17:06:42 INFO - [task 2018-08-21T17:06:42.641Z] 17:06:42 INFO - @pytest.mark.parametrize("action_type", ["none", "key", "pointer"]) [task 2018-08-21T17:06:42.641Z] 17:06:42 INFO - def test_pause_positive_integer(session, action_type): [task 2018-08-21T17:06:42.641Z] 17:06:42 INFO - for valid_duration in [0, 1]: [task 2018-08-21T17:06:42.642Z] 17:06:42 INFO - actions = [{ [task 2018-08-21T17:06:42.642Z] 17:06:42 INFO - "type": action_type, [task 2018-08-21T17:06:42.642Z] 17:06:42 INFO - "id": "foobar", [task 2018-08-21T17:06:42.643Z] 17:06:42 INFO - "actions": [{ [task 2018-08-21T17:06:42.643Z] 17:06:42 INFO - "type": "pause", [task 2018-08-21T17:06:42.644Z] 17:06:42 INFO - "duration": valid_duration [task 2018-08-21T17:06:42.645Z] 17:06:42 INFO - }] [task 2018-08-21T17:06:42.645Z] 17:06:42 INFO - }] [task 2018-08-21T17:06:42.645Z] 17:06:42 INFO - response = perform_actions(session, actions) [task 2018-08-21T17:06:42.645Z] 17:06:42 INFO - > assert_success(response) [task 2018-08-21T17:06:42.645Z] 17:06:42 INFO - [task 2018-08-21T17:06:42.645Z] 17:06:42 INFO - action_type = 'pointer' [task 2018-08-21T17:06:42.646Z] 17:06:42 INFO - actions = [{'actions': [{'duration': 0, 'type': 'pause'}], 'id': 'foobar', 'type': 'pointer'}]
Assignee | ||
Comment 106•6 years ago
|
||
(In reply to Andreea Pavel [:apavel] from comment #105) > [task 2018-08-21T17:06:42.639Z] 17:06:42 INFO - TEST-UNEXPECTED-FAIL | > /webdriver/tests/actions/bounds.py | test_pause_positive_integer[pointer] - > AssertionError: invalid argument (400): missing field `parameters` at line 1 > column 95 Hm, so from the WebDriver spec: https://w3c.github.io/webdriver/#dfn-process-pointer-parameters As I read it the parameters are indeed optional, and default to `pointerType = mouse`. That said, the patch is currently written to have that as required field. Thanks to Andreas for adding this test yesterday, so that we found this problem. I will fix it with my update of the patch.
Assignee | ||
Comment 107•6 years ago
|
||
(In reply to Andreea Pavel [:apavel] from comment #104) > Push that started the failures: > https://treeherder.mozilla.org/#/jobs?repo=mozilla- > inbound&revision=2fa2975f97e3e81f501aa982ab224ac470683f51 Sorry, but I'm not sure why the BR job wasn't run automatically. I will make sure to explicitely select it when running `mach try fuzzy` in the future. > Failure log: > https://treeherder.mozilla.org/logviewer.html#?job_id=195092647&repo=mozilla- > inbound&lineNumber=43221 The issue here is with the test `test_json_frame_id_webelement`, where we check serialization, and deserialization. The latter failed because it was missing the `deserialize_with` Serde attribute on the `Element` field. I will re-add this custom deserialization code for now, but given that it didn't seem to cause any failure in wdspec, the code path might not be used. Anyway, I would like to leave it in for now, and it's removal has to be reconsidered together with bug 1481776.
Flags: needinfo?(hskupin)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 114•6 years ago
|
||
Andreas, can you please check the following changes since your last review? https://reviewboard.mozilla.org/r/182200/diff/19-20/ I will also trigger a new try build so that we can be sure all will go well with the landing of the patch this time.
Flags: needinfo?(ato)
Assignee | ||
Comment 116•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #107) > I will re-add this custom deserialization code for now, but given that it > didn't seem to cause any failure in wdspec, the code path might not be used. > Anyway, I would like to leave it in for now, and it's removal has to be > reconsidered together with bug 1481776. This was actually a bad idea, and I will have to revert this. Also I remember now why I removed the custom deserialization code for FrameId. Reason is that this is not necessary for the WebDriver protocol, and if provided will cause failures when switching frames. See: https://treeherder.mozilla.org/#/jobs?repo=try&revision=948a5000d81293552b83028f9271a007cb64cee5&selectedJob=195253650 What needs to remain is the custom serialization code because that's necessary for Marionette, and will be removed with the mentioned bug. As such I will revert those changes, and update the test to only test the serialization of the FrameId.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 122•6 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3cc02142bee0 [wdspec] Mark all tests in new_session/merge.py to pass. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2c542df6fd [geckodriver] Switch webdriver crate from rustc_serialize to serde. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/bc455055ef6b [geckodriver] Apply rustfmt on other webdriver modules. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/c1df1c2e46f6 [geckodriver] Switch geckodriver crate from rustc_serialize to serde. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/559f9bc81c99 [geckodriver] Apply rustfmt other geckodriver modules. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/2cc9cac00814 Update vendored Rust crates. r=ato
![]() |
||
Comment 123•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3cc02142bee0 https://hg.mozilla.org/mozilla-central/rev/7f2c542df6fd https://hg.mozilla.org/mozilla-central/rev/bc455055ef6b https://hg.mozilla.org/mozilla-central/rev/c1df1c2e46f6 https://hg.mozilla.org/mozilla-central/rev/559f9bc81c99 https://hg.mozilla.org/mozilla-central/rev/2cc9cac00814
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee | ||
Comment 124•6 years ago
|
||
Dave and Raphael, because of the substantial changes in webdriver and geckodriver we would like to get some feedback from consumers of geckodriver if any kind of regression is still around and needs to be fixed before the next geckodriver release. Given that both you run a couple of projects which make use of geckodriver, your feedback is welcome. The geckodriver binary to test you can find inside the `common.tests` archive for your platform at https://archive.mozilla.org/pub/firefox/nightly/2018/09/2018-09-11-22-43-01-mozilla-central/. Thanks in advance.
Flags: needinfo?(rpierzina)
Flags: needinfo?(dave.hunt)
Assignee | ||
Comment 125•6 years ago
|
||
I got feedback from Alexei who run the test suite of the Selenium Java bindings with the latest nightly build of geckodriver, and he didn't see any other regression! I ran the tests for the Python bindings myself, and also didn't see a new failure.
Comment 126•6 years ago
|
||
I ran the redash-ui-tests integration tests [1] against Firefox 62.0 and geckodriver 0.21.0 (1169e8a4ca2b 2018-09-12 01:12 +0300) [2] and all tests pass [3]. [1]: https://github.com/mozilla/redash-ui-tests/pull/76 [2]: https://github.com/mozilla/redash-ui-tests/pull/76/files#diff-18ddb9a8a28b76b3c911b4039bcdfe17R9 [3]: https://circleci.com/gh/mozilla/redash-ui-tests/319 Hope this helps!
Flags: needinfo?(rpierzina)
Comment 127•6 years ago
|
||
I ran the Mozillians test against Firefox 62.0 and geckodriver 0.21.0 and all tests passed!
Flags: needinfo?(dave.hunt)
You need to log in
before you can comment on or make changes to this bug.
Description
•