Run Chrome tests in travis #14

equalsraf · 2018-01-30T19:28:32Z

Split the travis tests from #9.

Right now this is the same as #9 with a two commits on top to actually test with chrome, just so we can verify #9 works as intended.

TODO:

See if we can run tests without sudo, by disabling the sandbox Chrome not starting correctly travis-ci/travis-ci#7150 (comment)
Find a way to write driver independent tests - macros seem like an option but I have yet to find one way that does not required nightly
check if we can remove the addon lines or the package lines in travis 11aedcf#r27204188
update README

equalsraf · 2018-01-31T02:11:34Z

Or maybe something like feature gates for driver specific tests, https://www.reddit.com/r/rust/comments/3i1nki/how_to_skip_expensive_tests_with_cargo_test/ but it does not seem ideal either.

fluffysquirrels

A few small changes to capabilities API, but this is looking good.

Adding a NewSessionCmd to Driver.session() is a (worthwhile) breaking change, so this release will need to be published as v0.2.0.

fluffysquirrels · 2018-01-31T11:32:56Z

src/messages.rs

 }

 impl NewSessionCmd {
    pub fn new() -> Self {
+        let mut capabilities: Capabilities = Default::default();
+        capabilities.alwaysMatch.insert("goog:chromeOptions".to_string(), json!({"w3c": true}));


Can we push goog:chromeOptions down into a manual implementation of Default on struct Capabilities? That seems a little less surprising to me and allows the user to create a session with complete control over capabilities if required.

fluffysquirrels · 2018-01-31T11:34:51Z

src/messages.rs

 #[derive(Serialize)]
 pub struct NewSessionCmd {
-    capabilities: JsonValue,
+    capabilities: Capabilities,
 }

 impl NewSessionCmd {
    pub fn new() -> Self {


Take a Capabilities value as an argument here, I think.

If the concern is breaking changes then we can also remove new() and add it at a later time, and just leave default.

fluffysquirrels · 2018-01-31T11:45:18Z

src/messages.rs

        }
    }

 // TODO firefox specifc prefs
 // [moz:firefoxOptions][prefs][name] = value;

+    pub fn capability(&mut self, name: &str, value: Option<JsonValue>) {


Can we move this setter to struct Capabilities and rename it to always, always_match or similar? The capabilities spec describes alwaysMatch and firstMatch here, which we may add later.

I really like passing the value as an Option<_> to allow insert or remove with one method. 👍

Other possibilities here are

a method to merge capabilities

a build pattern of some kind, that either returns Self or &mut Self

fluffysquirrels · 2018-01-31T11:49:56Z

src/lib.rs

@@ -84,8 +85,8 @@ pub trait Driver {
    /// The url used to connect to this driver
    fn url(&self) -> &str;
    /// Start a session for this driver
-    fn session(self) -> Result<DriverSession, Error> where Self : Sized + 'static {
-        DriverSession::create_session(Box::new(self))
+    fn session(self, params: &NewSessionCmd) -> Result<DriverSession, Error> where Self : Sized + 'static {


This is a (worthwhile) breaking change, so this release will need to be published as v0.2.0.

equalsraf · 2018-01-31T11:52:05Z

Most tests work, but there is definitely something going on with the property method.

DEBUG:webdriver_client: GET http://localhost:44183/session/01f04760a3ba5e9c331177972d4f9b74/element/81f43d82-70e4-4950-84ca-60e06fbfaadf/property/href
DEBUG:webdriver_client: result status: 404 Not Found
body: 'unknown command: session/01f04760a3ba5e9c331177972d4f9b74/element/81f43d82-70e4-4950-84ca-60e06fbfaadf/property/href'
thread 'chrome::element_attribute_and_property' panicked at 'Error getting property: JsonDecodeError(ErrorImpl { code: ExpectedSomeValue, line: 1, column: 1 })', libcore/result.rs:916:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
DEBUG:webdriver_client: DELETE http://localhost:44183/session/01f04760a3ba5e9c331177972d4f9b74
DEBUG:webdriver_client: result status: 200 OK
body: '{"value":null}'

Apparently this is not implemented

fluffysquirrels · 2018-01-31T11:58:23Z

Travis reports some build failures: https://travis-ci.org/fluffysquirrels/webdriver_client_rust/builds/335577419

@illicitonion proposed invoking the tests with --test-threads=1, which is now on master. If you merge from master (perhaps no rebase this time :) ) this might improve reliability.

See #9, #14.

fluffysquirrels · 2018-01-31T12:43:01Z

Find a way to write driver independent tests - macros seem like an option but I have yet to find one way that does not required nightly

Which bit requires rust nightly? Doesn't this work on stable?

equalsraf · 2018-01-31T12:44:00Z

Which bit requires rust nightly? Doesn't this work on stable?

It runs on stable. But my previous versions depended on concat_idents.

fluffysquirrels · 2018-01-31T13:01:28Z

Ah! Thanks for clarifying.

Perhaps we could have:

mod firefox {
  browser_tests!(setup_firefox)
}

mod chrome {
  browser_tests!(setup_chrome)
}

fn setup_firefox() {}
fn setup_chrome() {}

equalsraf · 2018-01-31T13:13:33Z

That would imply moving the test code inside of the macro definition i.e.

macro_rules! driver_tests {
    ( $modname:ident, $driver_fn:expr ) => {
        mod $modname {
        #[test]
        fn navigation() {
            let (server, sess) = setup();
            let page1 = server.url("/page1.html");
            sess.go(&page1).expect("Error going to page1");
            assert_eq!(&sess.get_current_url().expect("Error getting url [1]"), &page1, "Wrong URL [1]");

(...all the other tests ...)
        }
    }
}

driver_tests!(gecko, setup_gecko)
driver_tests!(chrome, setup_gecko)

It should work, might just be a bit hard to edit tests inside the macro wrapper. Maybe move driver generic tests to different file and do an include!.

equalsraf · 2018-01-31T15:56:04Z

Is fe47626 similar to what you had in mind for the macros?

I have a test for this behaviour, but I'll wait for fluffysquirrels#14 to merge before adding it.

fluffysquirrels · 2018-02-01T14:04:17Z

Macros: thanks for working out the definition we need, it's starting to look really good!

I thought about this a lot today and wrote up a draft for you below, with comments to explain my reasoning.

// I think a trait is a little easier to grok than reading the macro documentation
// to understand what expressions are required. Also easier to extend.
//
// The trait can also be re-used by tests for a single browser (i.e. not in the browser_tests macro)
trait TestBrowser : Debug {
    fn session() -> DriverSession { [...] }
    fn driver() -> Box<Driver> { [...] }
}

#[derive(Default, Debug)]
struct FirefoxTest;
impl TestBrowser for FirefoxTest { [...] }

#[derive(Default, Debug)]
struct ChromeTest;
impl TestBrowser for ChromeTest { [...] }

/// Tests defined in this macro are run once per browser. See the macro invocations below.
macro_rules! browser_tests {
    ($mod_name:ident, $browser_type:ty) => {
        mod $mod_name {
            // Single browser-specific function to grok.
            fn browser() -> Box<TestBrowser> { Box::new($browser_type::default()) }

            // The tests are tightly coupled to the per-browser functions / types, so I think I prefer
            // them right next to each other in one file instead of in a separate included file.
            //
            // "might just be a bit hard to edit tests inside the macro wrapper":
            // I think it's clear enough if you've seen it before.
            // FWIW, this sort of pattern is quite common in rust std
            // e.g. https://github.com/rust-lang/rust/blob/56733bc9f8302409a2b6110f422512923c878154/src/libcore/tests/num/mod.rs#L145-L197
            #[test]
            fn first() -> { [...] }
        }
    };
}

browser_tests!(firefox, FirefoxTest);
browser_tests!(chrome, ChromeTest);

equalsraf · 2018-02-01T17:55:52Z

Confirmed, removing firefox from packages and keeping the addon line installs firefox 58. Funny part is we could probably xvfb and run headless firefox (have not tested, but should work since ff 57) https://github.com/mozilla/geckodriver#mozfirefoxoptions.

equalsraf · 2018-02-01T18:26:20Z

For later reference on can emulate the missing property method in chrome using javascript and an element ref

let link = sess.find_element("#link_to_page_2", LocationStrategy::Css)
    .expect("Error finding element");

let href = sess.execute(ExecuteCmd {
    script: "return arguments[0].href;".to_owned(),
    // the browser will convert the json reference to a js reference
    args: vec![link.reference().unwrap()],
}).expect("Error executing script");
exec_json.as_str().unwrap();

fluffysquirrels · 2018-02-03T11:09:10Z

I'd love to see headless Firefox. The private project I have using this crate is actually going to be on a headless server.

The xvfb stuff is messier, especially the sleep 3 that's required in the .travis.yml config! I'll add a new ticket to work on headless Firefox.

fluffysquirrels · 2018-02-03T13:14:40Z

EDIT: Ignore this. My alternative would have allowed a user to try and set alwaysMatch to a JSON value that is not an object, which we don't want.

I'm working on #18 ("Use Firefox -headless"), and I want to add command line args.

I think this:

                session_params.extend_always_match(
                    json!({
                        "moz:firefoxOptions": {
                            "args": ["-headless"]
                        };
                    }));

is a little neater for the consumer than:

                session_params.extend_always_match(
                    "moz:firefoxOptions", json!({
                        "args": ["-headless"]
                    }));

It's fewer characters to type (especially when setting multiple capabilities), and the implementation could be simpler (just use the JSON merge algorithm, as you linked on this serde_json thread).

Thoughts?

fluffysquirrels · 2018-02-03T15:50:40Z

I've built on this work in branch ff-headless and issue #18.

The latest Travis build on the branch just passed, and I'm thinking of merging it soonish.

Happy to hear your comments.

Remove the new constructor in NewSessionCmd since it is not clear how its api will evolve. Instead provide an implementation for the Default trait.

Capture the capabilities object when creating a session and provide a method to get the browserName property. When using a preexisting session the capabilities object is not available and the browser_name() method returns None.

fluffysquirrels

I made changes very similar to these in #18. Sorry for the duplicate work.

equalsraf · 2018-02-03T15:59:45Z

Thoughts?

Primarily the implication of this change is that the argument to that function MUST be an object otherwisee it would clobber the alwaysMatch attribute turning it into a non object, which would be invalid under the spec.

I've pushed an example

    pub fn extend_always_match2(&mut self, new_attrs: JsonValue) -> &mut Self {
        if let JsonValue::Object(map) = new_attrs {
            for (k, v) in map.into_iter() {
                Self::merge_obj(self.capabilities.alwaysMatch
                    .entry(k)
                    .or_insert(JsonValue::Null), v);
            }
        } else {
            // ERROR: input is not an object
        }
        self
    }

In this one I just ignore the error though. Otherwise it seems to work.

equalsraf · 2018-02-03T16:43:40Z

Continuing on my previous comment, if extend_always_match takes a value instead of (name, value) pair then should the other methods follow the same convention? Its possible to Same issue as before with error handling.

    pub fn always_match(&mut self, capability: JsonValue) -> &mut Self {
        if let JsonValue::Object(b) = capability {
            for (k, v) in b.into_iter() {
                self.capabilities.alwaysMatch.insert(k, v);
            }
        }
        self
    }

equalsraf · 2018-02-03T16:53:59Z

Mmm, i cant settle on a merge strategy and naming. I'll remove the extend related commits and leave those for another PR.

fluffysquirrels · 2018-02-03T17:52:36Z

Yes, I eventually realised your way (key and value, not object) was much better for the same reason. I edited my comment but I'm not sure if Github notifies you about edits. Sorry for the confusion.

README changes look good. I'll probably merge all this when I sit down at my computer.

This looks great! It'll be very useful to have Chrome tests running in the CI build.

I feel it would be useful to have examples and/or tests that don't run in headless mode. I find this useful for exploratory testing. I may add a separate issue for those.

fluffysquirrels · 2018-02-06T11:08:00Z

Hey, this turned out really well, thanks for all your work!

I've merged it with my Firefox headless stuff and will push to master then publish to crates.io when I get the build on my branch to pass.

fluffysquirrels · 2018-02-06T11:10:28Z

One small point that I updated in my branch but forgot to call out in your review.

I prefer this:

let cmd = NewSessionCmd::default();

to this:

let cmd: NewSessionCmd = Default::default();

Less typing, and I find it a little more straightforward, coming from an OO background.

Add travis settings to run tests with the stable version of chrome, a script to download chromedriver is available in bin/download_chromedriver. To avoid problems with the travis container the tests run in headless mode without the chrome sandbox: travis-ci/travis-ci#7150 (comment) To make the tests generic over different browsers they were wrapped in a macro whose purpose is to generate test modules for different drivers (currently chrome and firefox). At runtime tests can determine the driver they are using with the function test_browser(). It returns an enum that identifies the type of driver in use and provides constructors for the driver or session. Runtime checks for the browser being executed either use this enum or the browser name. Some loose ends are left unaddressed - we can skip a test at runtime but there is no way to indicate this in the test output, one just returns - part of the `execute` test is being skipped for chrome, the error strings and types are very different from firefox - Tests using the property method skipped in chrome because this is not implemented https://bugs.chromium.org/p/chromedriver/issues/detail?id=1936

equalsraf · 2018-02-06T11:23:41Z

Awesome, I've pushed the NewSessionCmd change anyway. should I close this one?

fluffysquirrels · 2018-02-06T11:27:22Z

Merged.

equalsraf mentioned this pull request Jan 30, 2018

Support for other browsers #9

Merged

equalsraf force-pushed the chrome-tests branch 2 times, most recently from 3987022 to dba3984 Compare January 31, 2018 11:28

fluffysquirrels requested changes Jan 31, 2018

View reviewed changes

fluffysquirrels pushed a commit that referenced this pull request Jan 31, 2018

Added note to Cargo.toml to make the next version v0.2.

c5a4a67

See #9, #14.

equalsraf force-pushed the chrome-tests branch from 249ffe6 to fd87258 Compare January 31, 2018 12:47

equalsraf force-pushed the chrome-tests branch from 2b4549f to fe47626 Compare January 31, 2018 14:04

illicitonion added a commit to illicitonion/webdriver_client_rust that referenced this pull request Feb 1, 2018

Support launching multiple Chrome DriverSessions

ae302be

I have a test for this behaviour, but I'll wait for fluffysquirrels#14 to merge before adding it.

illicitonion mentioned this pull request Feb 1, 2018

Support launching multiple Chrome DriverSessions #17

Closed

equalsraf force-pushed the chrome-tests branch from fe47626 to b821dd2 Compare February 1, 2018 17:36

equalsraf force-pushed the chrome-tests branch 2 times, most recently from efa2391 to 40132e3 Compare February 1, 2018 21:16

fluffysquirrels mentioned this pull request Feb 3, 2018

Use Firefox -headless #18

Open

4 tasks

fluffysquirrels approved these changes Feb 3, 2018

View reviewed changes

equalsraf added 2 commits February 3, 2018 15:53

Add api to configure capabilities

afd57d7

Remove the new constructor in NewSessionCmd since it is not clear how its api will evolve. Instead provide an implementation for the Default trait.

Add method to get browser name

04b1881

Capture the capabilities object when creating a session and provide a method to get the browserName property. When using a preexisting session the capabilities object is not available and the browser_name() method returns None.

fluffysquirrels reviewed Feb 3, 2018

View reviewed changes

equalsraf force-pushed the chrome-tests branch from 4334a7f to a6d115c Compare February 3, 2018 15:56

equalsraf force-pushed the chrome-tests branch 2 times, most recently from 2127793 to 984556b Compare February 3, 2018 17:25

equalsraf force-pushed the chrome-tests branch from 984556b to bfcdfaa Compare February 6, 2018 11:21

equalsraf added 3 commits February 6, 2018 11:21

Remove unused firefox pkg from .travis.yml

74b138f

Update README on chromedriver

bfcdfaa

fluffysquirrels closed this Feb 6, 2018

Run Chrome tests in travis #14

Run Chrome tests in travis #14

Uh oh!

Conversation

equalsraf commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

equalsraf commented Jan 31, 2018

Uh oh!

fluffysquirrels left a comment

Choose a reason for hiding this comment

Uh oh!

fluffysquirrels Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

fluffysquirrels Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

equalsraf Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

fluffysquirrels Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

equalsraf Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

fluffysquirrels Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

equalsraf commented Jan 31, 2018

Uh oh!

fluffysquirrels commented Jan 31, 2018

Uh oh!

fluffysquirrels commented Jan 31, 2018

Uh oh!

equalsraf commented Jan 31, 2018

Uh oh!

fluffysquirrels commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

equalsraf commented Jan 31, 2018

Uh oh!

equalsraf commented Jan 31, 2018

Uh oh!

fluffysquirrels commented Feb 1, 2018

Uh oh!

equalsraf commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

equalsraf commented Feb 1, 2018

Uh oh!

fluffysquirrels commented Feb 3, 2018

Uh oh!

fluffysquirrels commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fluffysquirrels commented Feb 3, 2018

Uh oh!

fluffysquirrels left a comment

Choose a reason for hiding this comment

Uh oh!

equalsraf commented Feb 3, 2018

Uh oh!

equalsraf commented Feb 3, 2018

Uh oh!

equalsraf commented Feb 3, 2018

Uh oh!

fluffysquirrels commented Feb 3, 2018

Uh oh!

fluffysquirrels commented Feb 6, 2018

Uh oh!

fluffysquirrels commented Feb 6, 2018

Uh oh!

equalsraf commented Feb 6, 2018

Uh oh!

fluffysquirrels commented Feb 6, 2018

Uh oh!

Uh oh!

equalsraf commented Jan 30, 2018 •

edited

Loading

fluffysquirrels commented Jan 31, 2018 •

edited

Loading

equalsraf commented Feb 1, 2018 •

edited

Loading

fluffysquirrels commented Feb 3, 2018 •

edited

Loading