Fix wdspec tests using new_session fixture #7461

carlosgcampos · 2017-09-25T07:40:34Z

Tests are passing capabilities that should be used when creating the
session, but they are passed directly as body of new session command,
without adding them to a capabilities object. This is also ignoring all
other configuration capabilities, preventing the browser from being
launched in cases where the browser binary is passed to the driver as a
capability. We should use the configuration capabilities, overwrite
the ones given by the test and pass them to the Session at construction.
This way we can use Session::start() instead of using send_command()
directly.

ghost · 2017-09-25T08:11:15Z

Build ERRORED

Started: 2017-10-16 15:35:27
Finished: 2017-10-16 16:24:25

Failing Jobs

firefox:nightly
chrome:unstable

View more information about this build on:

andreastt

Barring the fact that the documentation for overwrite_dictionary should be fixed up, this is fine.

andreastt · 2017-09-25T13:28:08Z

webdriver/tests/support/merge_dictionaries.py

@@ -24,6 +24,27 @@ def merge_dictionaries(first, second):

    return result

+def overwrite_dictionary(first, second):
+    """Given two dictionaries, create a third that defines all specified
+    key/value pairs. This merge_dictionaries is performed "deeply" on any nested


Reference to merge_dictionaries?

Oops, copy paste.

mjzffr · 2017-09-26T01:15:19Z

webdriver/tests/support/fixtures.py

-        # TODO: merge in some capabilities from the confguration capabilities
-        # since these might be needed to start the browser
-        value = _session.send_command("POST", "session", body=body)
+                                     capabilities=overwrite_dictionary(configuration["capabilities"], capabilities))


@carlosgcampos @andreastt FYI, I needed to change this to capabilities={"alwaysMatch": caps} to get it working locally. (Where caps is the result of overwrite_dictionary(...))

hmm, tests are already passing {"alwaysMatch": caps} to new_session, you shouldn't need to add it again.

Wait, not all the tests are doing the same thing, it seems new_session tests are passing {"capabilities": caps}. I'll check those and update the patch, thanks @mjzffr

mjzffr

Could you also provide a default value for capabilities? e.g. {"alwaysMatch": configuration["capabilities"]}

It would be useful if a test could use new_session() to mean that we just want the configuration capabilities. This comes up in subtests that aren't testing session creation but just need a new session for each subtest.

@mjzffr

Per @mjzffr’s comment.

carlosgcampos · 2017-09-28T09:58:11Z

Good idea, updated the patch to allow using new_session with no arguments

andreastt

r+

mjzffr

Thanks!

carlosgcampos · 2017-09-29T06:58:09Z

Thank you guys, can this be merged now, then?

andreastt · 2017-09-30T15:13:10Z

The CI appears to be failing because the tests have now become unstable as a result of this change.

carlosgcampos · 2017-10-01T08:22:13Z

How can I see what is failing? I don't see anything useful here

andreastt · 2017-10-02T12:54:17Z

Neither can I. I don’t really understand how to debug test stability failures in those Travis jobs. I’ve retriggered the build a couple of times to no avail…

whimboo · 2017-10-04T17:08:44Z

The stability tests fail because there is actually some kind of crash or disconnect in Firefox. You can see it here: https://travis-ci.org/w3c/web-platform-tests/jobs/280747953#L7012-L7013

It happens directly after http://web-platform.test:8000/webdriver/tests/support/inline.py?doc=%3Ca+href%3D%23+id%3DlinkText%3Efull+link+text%3C%2Fa%3E&content-type=text%2Fhtml%3Bcharset%3Dutf-8 got finished.

As it looks like a Firefox process might be left behind, which prevents Marionette from setting up the server socket for all the following tests. That's why the server socket failure.

carlosgcampos · 2017-10-15T07:45:24Z

@whimboo do you think this is caused by this patch? or a problem in the bot? or maybe the firefox version used in the bots?

whimboo · 2017-10-16T06:01:13Z

I cannot see that the issue should be related to your patch. Also because no capabilities should have been changed too. Did someone actually retrigger this failing job once or multiple times? How often does it fail? Given that it's using Firefox Nightly it might have been an issue on that day. So please lets get this job retriggered, so we can see if it still fails.

andreastt · 2017-10-16T15:33:41Z

Yes, I’ve re-triggered it five or six times with the same result.

jgraham · 2017-10-16T15:57:10Z

I think this is theoretically problematic because adding extra capabilities can invalidate the tests. I'm not sure what a good workaround is though, given that some capabilities are required to start the browser.

carlosgcampos · 2017-10-17T07:24:28Z

I think this patch only changes the behavior for tests not passing a valid capabilities dictionary, because when receiving a valid capabilities dict we use that as the body of the new session command like current code does. So, the ones passing {"alwaysMatch": } might be the ones causing the failures. Those tests are using prompts, that I think are not implemented by geckodriver (right?), so with current code they are probably not executed, because the session is rejected due to the invalid capabilities. With this patch, those tests are correctly executed, but prompts are not actually handled. Could it be that we leave a firefox instance with an unhandled prompt or something? Could we try skipping those tests for firefox driver only?

jgraham · 2017-10-17T12:14:58Z

The failures on Travis are mostly just the tests timing out. I ran locally and got the following unstable results:

## Unstable results ##

|                            Test                           |                            Subtest                             |          Results           | Messages |
|-----------------------------------------------------------|----------------------------------------------------------------|----------------------------|----------|
| `/webdriver/tests/navigation/current_url.py`              | `current_url.py::test_get_current_url_after_modified_location` | **FAIL: 3/10, PASS: 7/10** |          |
| `/webdriver/tests/sessions/new_session/default_values.py` | `default_values.py::test_missing_always_match`                 | **FAIL: 1/10, PASS: 9/10** |          |

I am particularly concerned with the default_values.py::test_missing_always_match test; I think that test just doesn't do what's expected in this case.

I think a more reasonable approach would be to make a fixture that could add default capabilities to a dictionary and put the tests that use new_session directly in control of adding these in the desired place (if any).

jgraham · 2017-10-17T12:16:30Z

(note that I didn't check explicitly for regressions with this patch applied, but at a glance there are some, and we will need to check carefully before landing).

carlosgcampos · 2017-10-17T12:45:35Z

But default_values.py::test_missing_always_match is passing '{"capabilities": {"firstMatch": [{}]}}' so in that case, the given dictionary is used directly as the body the new session command, there shouldn't be a change in behavior unless I'm missing something.

jgraham · 2017-10-17T12:51:07Z

Oh right. But presumably then this simply won't fix that case (which is a valid new session).

I still think this implementation is too magical and that it would be better to make the tests pass this information themselves.

carlosgcampos · 2017-10-17T13:03:01Z

This patch doesn't try to fix the tests passing { "capabilities": ... }, since the behavior doesn't change for those, but the ones passing { "alwaysMatch" : ... }. I'm fine with adding a different fixture for these one, though.

whimboo · 2017-10-18T08:39:43Z

Could we temporarily run a travis job with this patch applied by using trace logging of Marionette, and maybe a debug build of Firefox? Both should give us way more debugging information to finally get a sense of what's going on here.

@jgraham if you are still able to reproduce locally maybe you could do it and upload the trace log?

jgraham · 2017-10-18T10:08:10Z

@whimboo I don't think there's any mystery about what's going on here. The problem is that if the browser isn't on the $PATH we need to pass in the browser configuration capabilities to find it, which we currently don't do for the new session tests. The remaining question is how to add those to the payload in these cases where that payload is the thing under test. This patch tries to do it implicitly, but I strongly believe we should make it explicit in the tests.

carlosgcampos · 2017-10-18T10:21:55Z

I'm going to try the explicit approach as soon as I find some time to work on this, hopefully later today.

jgraham · 2017-10-19T13:33:17Z

webdriver/tests/sessions/new_session/invalid_capabilities.py

@@ -83,6 +88,11 @@ def test_invalid_values(new_session, body, key, value):
                                  lambda key, value: {"firstMatch": [{key: value}]}])
 @pytest.mark.parametrize("key", invalid_extensions)
 def test_invalid_extensions(new_session, body, key):


You need to change the signature here, right?

jgraham · 2017-10-19T13:33:59Z

webdriver/tests/sessions/new_session/merge.py

@@ -24,17 +29,17 @@ def test_platform_name(new_session, platform_name, body):


 @pytest.mark.parametrize("key,value", invalid_merge)
-def test_merge_invalid(new_session, key, value):
+def test_merge_invalid(new_session, add_browser_capabilites, key, value):


This isn't strictly needed, but I guess it's better this way.

Tests are passing capabilities that should be used when creating the session, but they are passed directly as body of new session command, without adding them to a capabilities object. This is also ignoring all other configuration capabilities, preventing the browser from being launched in cases where the browser binary is passed to the driver as a capability. This patch adds a new fixture add_browser_capabilites, that is used by tests using new_session to provide the browser capabilities and optionally add others.

jgraham · 2017-10-24T12:32:37Z

I ran the stability checker locally and this doesn't seem to introduce any new problems, so merging.

wpt-pr-bot added webdriver wg-testing_browser labels Sep 25, 2017

wpt-pr-bot requested review from andreastt, AutomatedTester, lukeis, mjzffr and shs96c September 25, 2017 07:40

andreastt requested changes Sep 25, 2017

View reviewed changes

carlosgcampos force-pushed the wd-new-session branch from 3cf48ab to 6006a16 Compare September 25, 2017 15:38

andreastt previously approved these changes Sep 25, 2017

View reviewed changes

mjzffr reviewed Sep 26, 2017

View reviewed changes

carlosgcampos force-pushed the wd-new-session branch from 6006a16 to 77eab20 Compare September 26, 2017 07:44

mjzffr requested changes Sep 27, 2017

View reviewed changes

andreastt mentioned this pull request Sep 27, 2017

Set capabilities in new_session #7513

Closed

carlosgcampos force-pushed the wd-new-session branch from 77eab20 to 4b41e1f Compare September 28, 2017 06:55

andreastt approved these changes Sep 28, 2017

View reviewed changes

mjzffr approved these changes Sep 28, 2017

View reviewed changes

wpt-pr-bot requested a review from kereliuk October 4, 2017 17:09

mjzffr mentioned this pull request Oct 16, 2017

doubleClick() does not trigger 'dblclick' event mozilla/geckodriver#661

Closed

2 tasks

carlosgcampos force-pushed the wd-new-session branch from 4b41e1f to b3c6e31 Compare October 19, 2017 13:02

jgraham requested changes Oct 19, 2017

View reviewed changes

carlosgcampos force-pushed the wd-new-session branch from b3c6e31 to bd61ca8 Compare October 19, 2017 13:41

carlosgcampos force-pushed the wd-new-session branch from bd61ca8 to 8ba8a80 Compare October 20, 2017 07:53

jgraham approved these changes Oct 20, 2017

View reviewed changes

jgraham merged commit 09c7045 into web-platform-tests:master Oct 24, 2017

carlosgcampos deleted the wd-new-session branch October 24, 2017 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wdspec tests using new_session fixture #7461

Fix wdspec tests using new_session fixture #7461

carlosgcampos commented Sep 25, 2017 •

edited by wpt-pr-bot

Loading

ghost commented Sep 25, 2017 •

edited by ghost

Loading

andreastt left a comment

andreastt Sep 25, 2017

carlosgcampos Sep 25, 2017

mjzffr Sep 26, 2017

carlosgcampos Sep 26, 2017

carlosgcampos Sep 26, 2017

mjzffr left a comment

carlosgcampos commented Sep 28, 2017

andreastt left a comment

mjzffr left a comment

carlosgcampos commented Sep 29, 2017

andreastt commented Sep 30, 2017

carlosgcampos commented Oct 1, 2017

andreastt commented Oct 2, 2017

whimboo commented Oct 4, 2017

carlosgcampos commented Oct 15, 2017

whimboo commented Oct 16, 2017

andreastt commented Oct 16, 2017

jgraham commented Oct 16, 2017

carlosgcampos commented Oct 17, 2017

jgraham commented Oct 17, 2017

jgraham commented Oct 17, 2017

carlosgcampos commented Oct 17, 2017

jgraham commented Oct 17, 2017

carlosgcampos commented Oct 17, 2017

whimboo commented Oct 18, 2017

jgraham commented Oct 18, 2017

carlosgcampos commented Oct 18, 2017

jgraham Oct 19, 2017

carlosgcampos Oct 19, 2017

jgraham Oct 19, 2017

jgraham commented Oct 24, 2017

Fix wdspec tests using new_session fixture #7461

Fix wdspec tests using new_session fixture #7461

Conversation

carlosgcampos commented Sep 25, 2017 • edited by wpt-pr-bot Loading

ghost commented Sep 25, 2017 • edited by ghost Loading

Build ERRORED

Failing Jobs

andreastt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjzffr left a comment

Choose a reason for hiding this comment

carlosgcampos commented Sep 28, 2017

andreastt left a comment

Choose a reason for hiding this comment

mjzffr left a comment

Choose a reason for hiding this comment

carlosgcampos commented Sep 29, 2017

andreastt commented Sep 30, 2017

carlosgcampos commented Oct 1, 2017

andreastt commented Oct 2, 2017

whimboo commented Oct 4, 2017

carlosgcampos commented Oct 15, 2017

whimboo commented Oct 16, 2017

andreastt commented Oct 16, 2017

jgraham commented Oct 16, 2017

carlosgcampos commented Oct 17, 2017

jgraham commented Oct 17, 2017

jgraham commented Oct 17, 2017

carlosgcampos commented Oct 17, 2017

jgraham commented Oct 17, 2017

carlosgcampos commented Oct 17, 2017

whimboo commented Oct 18, 2017

jgraham commented Oct 18, 2017

carlosgcampos commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgraham commented Oct 24, 2017

carlosgcampos commented Sep 25, 2017 •

edited by wpt-pr-bot

Loading

ghost commented Sep 25, 2017 •

edited by ghost

Loading