Closed Bug 1396821 Opened 7 years ago Closed 6 years ago

Use serde in place of rustc_serialize

Categories

(Testing :: geckodriver, enhancement, P1)

Product:
Testing
Component:
geckodriver
Version:
Version 3
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
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)

Bug 1396821 - [geckodriver] Switch webdriver crate from rustc_serialize to serde.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1396821 - [geckodriver] Switch geckodriver crate from rustc_serialize to serde.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1396821 - [wdspec] Mark all tests in new_session/merge.py to pass.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1396821 - Update vendored Rust crates.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1396821 - [geckodriver] Apply rustfmt on other webdriver modules.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1396821 - [geckodriver] Apply rustfmt other geckodriver modules.
59 bytes, 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.
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());
>> }
jgraham:

> Ah, that does look useful indeed. Thanks!
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.
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),
>>                 })
>>             }
>>         }
>>     }
>> }
whimboo:

> @jgraham what do you think about the proposal from @dtolnay?
jonhoo:

> It'd be great to see some progress on this so we can drop the
> dependency on the deprecated rustc-serialize
Priority: -- → P3
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
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
Attached patch Use serde [de]serialize (obsolete) — — Splinter Review 

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
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.

    
    

    
      
  


  

    
    

    
      
  


  

        Attachment #8910472 -
        Attachment is obsolete: true

        Attachment #8910474 -
        Attachment is obsolete: true
Depends on: 1402104

    
    

    
  
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.

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

        Attachment #8910912 -
        Attachment is obsolete: true

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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.
Depends on: 1442028

    
    

    
      
  

    
    

    
      
  

        Attachment #8912283 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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.

    
    

    
  
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)

    
    

    
  
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)

    
    

    
  
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.

    
    

    
      
  


  

        Attachment #8910911 -
        Attachment is obsolete: true

        Attachment #8910910 -
        Attachment is obsolete: true
Depends on: 1469752

    
    

    
      
  


  

    
    

    
      
  

    
    

    
  
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.
Blocks: 1469752
No longer depends on: 1469752

    
    

    
      
  

        Attachment #8988570 -
        Attachment is obsolete: true

    
    

    
  
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.
Depends on: 1473341

    
    

    
      
  

    
    

    
  
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.

    
    

    
  
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.
Blocks: 1481776
Depends on: 1482042

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  
Priority: P3 → P1

    
    

    
  
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`.

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

        Attachment #8998754 -
        Attachment is obsolete: true

    
    

    
  
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.
Depends on: 1483559

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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),
Depends on: 1483801

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

        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)

    
    

    
  
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.
Blocks: 1483559
No longer depends on: 1483559

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
Comment on attachment 9001256 [details]
Bug 1396821 - Update vendored Rust crates.

https://reviewboard.mozilla.org/r/261696/#review269624

        Attachment #9001256 -
        Flags: review?(ato) → 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+

    
    

    
  
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.

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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 "{:?}".

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
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.

    
    

    
  
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 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 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)

    
    

    
  
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.

    
    

    
  
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.

        Attachment #8997382 -
        Flags: review?(james)

    
    

    
  
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

    
    

    
  
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'}]

    
    

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

    
    

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

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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)

    
    

    
  
LGTM
Flags: needinfo?(ato)

    
    

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

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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
No longer depends on: 1485397
Blocks: 1466818

    
  
Blocks: 1396824
Depends on: 1486793
Depends on: 1489141

    
    

    
  
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)

    
    

    
  
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.

    
    

    
  
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)

    
    

    
  
I ran the Mozillians test against Firefox 62.0 and geckodriver 0.21.0 and all tests passed!
Flags: needinfo?(dave.hunt)

    
    

    
  
Thank you both! That sounds perfect then.
Depends on: 1491751
Depends on: 1493149
Blocks: 1493095
Depends on: 1494613
Depends on: 1494617
Depends on: 1494637
Depends on: 1503804
Depends on: 1507283
Depends on: 1517196

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: