Add test for response headers #14074

christian-bromann · 2018-11-15T10:18:31Z

The WebDriver spec defines the response header in the processing modal section as follows:

Set the response’s header with name and value with the following values:

Content-Type
"application/json; charset=utf-8"
Cache-Control
"no-cache"

This patch adds tests for this case.

Question: the RFC defines header names as case insensitive. Should be lowercase header keys and values by default?

Note:: Geckodriver returns a wrong response value for content-type, so I created a bug here: https://bugzilla.mozilla.org/show_bug.cgi?id=1507428

jgraham · 2018-11-15T12:01:40Z

webdriver/tests/new_session/headers.py

@@ -0,0 +1,6 @@
+def test_missing_first_match(new_session, add_browser_capabilities):
+    response, _ = new_session({"capabilities": {"alwaysMatch": add_browser_capabilities({})}})
+    assert 'cache-control' in response.headers


I think you should lowercase the recieved values here.

@jgraham see PR question: shall we by default lowercase key and value pairs since headers are case insensitive?

webdriver/tests/new_session/headers.py

christian-bromann · 2018-11-15T12:29:35Z

@jgraham given the fact that this test should be applied to every response I moved the assertion into the Session class. I also converted header key/value pairs into lowercase per default.

andreastt · 2018-11-16T12:26:23Z

I sort of feel we should return headers unmodified as they are presented to us, and only convert them to lowercase during assertion.

andreastt · 2018-11-16T14:46:06Z

There also appears to be some test failures after one of the recent fixups:
https://travis-ci.org/web-platform-tests/wpt/builds/455470716?utm_source=github_status&utm_medium=notification

christian-bromann · 2018-11-16T17:22:24Z

@andreastt addressed outstanding issues

old

andreastt · 2018-11-16T19:42:39Z

Retriggered some of the jobs and dismissed @jgraham’s old review.

christian-bromann · 2018-11-16T20:14:28Z

Azure Pipeline is failing due to

Chrome not having cache-control in command response header
FF Nightly returning application/json; charset=utf8 in response header

continuous-integration/travis-ci/pr failing due to

FF Nightly returning application/json; charset=utf8 in response header

andreastt · 2018-11-16T20:21:18Z

@jgraham Aren’t those jobs supposed to run stability tests, like Travis?

jugglinmike · 2018-11-16T20:28:47Z

Yup, but only for tests that it recognizes as being somehow affected by the change set.

2018-11-16 17:23:10,943 - tc-run - INFO - Identifying tests affected in range 'HEAD^'...
2018-11-16 17:23:19,970 - tc-run - INFO - Identified 0 affected tests
2018-11-16 17:23:19,971 - tc-run - INFO - Quitting because no tests were affected.

jugglinmike · 2018-11-16T22:23:06Z

@jgraham given the fact that this test should be applied to every response I
moved the assertion into the Session class.

This library is used by the WPT infrastructure to run all the other conformance
tests in browsers that implement the W3C WebDriver protocol. Enforcing strict
conformance criteria here could have severe effects on the rest of the test
suite. For instance, it may be wrong for an implementation to omit the
'Content-Type' header, but if we assert its presence in these bindings, then
that implementation will fail every test in WPT. This is unavoidable in severe
cases, but in cases where the tool can be permissive (e.g. just assuming the
body is JSON), doing so gives folks a better picture of the issue: a failing
WebDriver spec test. That's why I recommend making assertions like these within
the WebDriver conformance test suite itself, as per your original commit.

If we want to make this assertion for every WebDriver test, we could:

Keep the asserts here and update wptrunner to ignore assert statements in
wptrunner's subprocess for reftests and testharness tests. In that case, we'd
also have to update the existing infrastructure assertions to ensure they
persist.
Add a generic "on response" hook to the library and attach an assertion in
the wdspec test suite's setup code

Although either would suffer from the problem above, just on a smaller scale.

andreastt · 2018-11-17T14:59:45Z

Or we could keep the checks in the client and emit a warning when the headers do not conform. I support the idea of moving the hard assertions to a separate test.

andreastt

See comment above.

christian-bromann · 2018-11-19T12:17:24Z

@andreastt @jugglinmike I moved the assertions into the assert_error and assert_success assertion helpers which are only used in the webdriver test harness. What do you think?

jugglinmike

Good idea! Unlike the alternatives we've been considering, this may allow some responses to go untested. Those cases seem rare, so I think the simplicity of this approach wins out.

As for the timeout reported by Taskcluster: unlike with the previous solution, the system is now capable of identifying the affected tests:

2018-11-19 12:17:10,747 - tc-run - INFO - Identifying tests affected in range 'HEAD^'...
2018-11-19 12:17:26,483 - tc-run - INFO - Identified 138 affected tests

That's a lot of tests, possibly enough to exceed our 2-hour limit, though the logs for Chrome are suspiciously short...

whimboo · 2019-01-28T21:37:16Z

The newly added assertion has failures:
https://tools.taskcluster.net/groups/cZgXyDz3T0S0m1Sh4X024w/tasks/Ub-qkzjxRoCtmPoz6V_PNg/runs/1/logs/public%2Flogs%2Flive.log#L96762

TypeError: assert_response_headers() takes exactly 2 arguments (1 given)

@christian-bromann can you please fix that? Thanks

christian-bromann · 2019-02-22T11:01:55Z

@whimboo done

FF still fails because content type is application/json; charset=utf8 instead of application/json; charset=utf-8
Chrome still fails as it doesn't have a cache control header

whimboo · 2019-02-26T15:50:44Z

I don't know why it fails with Firefox given that we shipped the fix for utf-8 with geckodriver 0.24.0. @jugglinmike do we still have to manually change the version number of the driver?

jugglinmike · 2019-02-26T16:50:09Z

Nope

whimboo · 2019-02-26T21:37:00Z

Oh wait. The Firefox job with the patch applied doesn't fail! What's failing is the verify job, which is killed by taskcluster after a runtime of 7200s.

christian-bromann · 2019-02-26T22:54:24Z

@whimboo would closing and reopening the PR help?

whimboo · 2019-02-27T07:53:23Z

Not sure. Running all the tests simply take that long. Someone might take a bit of time to check how long those verify tasks run in general, and maybe we have to bump up the maximum allowed time.

gsnedders · 2019-02-28T15:13:39Z

@whimboo I'm not sure we can while we're using the GitHub Taskcluster integration, but @jgraham would know better. There are a few cases where we end up running lots of tests, but I think the longer term plan is to do something smarter and dynamically shard them.

In either case, if you think this looks good, we should just force-merge this.

mustjab · 2019-02-28T20:42:18Z

@jgraham I see WPT WebDriver tests failing in Chrome on Windows and Linux runs with this change, and pass after reverting it locally. Anyone else seeing this?

  Γöé webdriver\tests\add_cookie\add.py:17:
  Γöé _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
  Γöé webdriver\tests\support\asserts.py:55: in assert_error
  Γöé     assert_response_headers(response.headers)
  Γöé _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
  Γöé
  Γöé headers = {'content-length': '3243', 'content-type': 'application/json; charset=utf-8'}
  Γöé
  Γöé     def assert_response_headers(headers):
  Γöé         """
  Γöé         Method to assert response headers for WebDriver requests
  Γöé
  Γöé         :param headers: dict with header data
  Γöé         """
  Γöé >       assert 'cache-control' in headers
  Γöé E       AssertionError: assert 'cache-control' in {'content-length': '3243', 'content-type': 'application/json; charset=utf-8'}
  Γöé
  Γöé headers    = {'content-length': '3243', 'content-type': 'application/json; charset=utf-8'}
  Γöé
  Γöö webdriver\tests\support\asserts.py:82: AssertionError

andreastt · 2019-02-28T20:50:06Z

@mustjab I think that would be expected, since chromedriver doesn’t return the mandated Cache-Control header.

See send a response in the spec.

mustjab · 2019-02-28T20:53:31Z

@andreastt So should we change the way we run these tests? We run them on a daily basis and at this moment most of the tests fail due to this error.

andreastt · 2019-02-28T21:13:17Z

Right, the assertion was added as a step to assert_success(), causing most—if not all!—of the wdspec tests to fail because the driver doesn’t return the mandated headers.

In order to avoid numerous workarounds, I think the best fix here is to insert a new line into https://cs.chromium.org/chromium/src/chrome/test/chromedriver/server/http_handler.cc?l=1125 and fix chromedriver. Alternatively we could back this change out temporarily to give you time to fix this.

mustjab · 2019-02-28T23:00:42Z

Thank you! I'll outreach to ChromeDriver team to fix this and for now, we'll run with this fix internally.

jgraham · 2019-03-01T13:19:27Z

I should have read the patch before merging it. I fully agree that this shouldn't be tested for every test but should be a specific set of tests or otherwise isolated from testing other features.

christian-bromann · 2019-03-01T13:36:00Z

@jgraham I will tweak the patch and make it so that it is handled in a single test instead of being part of all assertions.

whimboo · 2019-03-04T20:54:47Z

@jgraham, so we want to backout the already landed patch for a clean base? I don't think patching on top of the current patch is that great.

andreastt · 2019-03-04T21:50:37Z

If @mustjab is fixing chromedriver, I don’t necessarily see the need to move the assertion to separate test, nor a reason to back this out.

Hexcles · 2019-03-05T16:50:25Z

https://chromium-review.googlesource.com/c/chromium/src/+/1501997

wpt-pr-bot added infra webdriver wg-testing_browser labels Nov 15, 2018

wpt-pr-bot requested review from andreastt, AutomatedTester, gsnedders, jgraham, jugglinmike, mjzffr, shs96c and whimboo November 15, 2018 10:18

jgraham previously requested changes Nov 15, 2018

View reviewed changes

andreastt removed their request for review November 16, 2018 15:09

foolip mentioned this pull request Nov 16, 2018

Enable Azure Pipelines #13818

Closed

andreastt approved these changes Nov 16, 2018

View reviewed changes

andreastt requested changes Nov 17, 2018

View reviewed changes

christian-bromann mentioned this pull request Nov 19, 2018

Add Access-Control-Allow-Origin to response header w3c/webdriver#1362

Closed

jugglinmike approved these changes Nov 19, 2018

View reviewed changes

christian-bromann added 6 commits February 22, 2019 11:52

add test for response headers

8f07c37

change test name

7d9df7f

move header assertion to send_command method

df804bf

transform headers on assertion

b0c756e

moved assertion to webdriver test harness

e41a5cd

fixed assert_response_headers method

641546e

christian-bromann force-pushed the cb-header-checks branch from f207ac2 to 641546e Compare February 22, 2019 11:00

add back cache-control check

995787b

jgraham merged commit d4320a3 into web-platform-tests:master Feb 28, 2019

christian-bromann deleted the cb-header-checks branch February 28, 2019 15:59

marcoscaceres pushed a commit that referenced this pull request Jul 23, 2019

Add test for response headers (#14074)

8753260

stephenmcgruer mentioned this pull request Sep 15, 2020

Python3: Accommodate both highcase and lowcase for response header key #25039

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for response headers #14074

Add test for response headers #14074

christian-bromann commented Nov 15, 2018 •

edited

Loading

jgraham Nov 15, 2018

christian-bromann Nov 15, 2018

christian-bromann commented Nov 15, 2018

andreastt commented Nov 16, 2018

andreastt commented Nov 16, 2018

christian-bromann commented Nov 16, 2018

andreastt commented Nov 16, 2018

christian-bromann commented Nov 16, 2018

andreastt commented Nov 16, 2018

jugglinmike commented Nov 16, 2018

jugglinmike commented Nov 16, 2018

andreastt commented Nov 17, 2018

andreastt left a comment

christian-bromann commented Nov 19, 2018

jugglinmike left a comment

whimboo commented Jan 28, 2019

christian-bromann commented Feb 22, 2019

whimboo commented Feb 26, 2019

jugglinmike commented Feb 26, 2019

whimboo commented Feb 26, 2019

christian-bromann commented Feb 26, 2019

whimboo commented Feb 27, 2019

gsnedders commented Feb 28, 2019

mustjab commented Feb 28, 2019

andreastt commented Feb 28, 2019

mustjab commented Feb 28, 2019

andreastt commented Feb 28, 2019

mustjab commented Feb 28, 2019

jgraham commented Mar 1, 2019

christian-bromann commented Mar 1, 2019

whimboo commented Mar 4, 2019

andreastt commented Mar 4, 2019

Hexcles commented Mar 5, 2019

Add test for response headers #14074

Add test for response headers #14074

Conversation

christian-bromann commented Nov 15, 2018 • edited Loading

jgraham Nov 15, 2018

Choose a reason for hiding this comment

christian-bromann Nov 15, 2018

Choose a reason for hiding this comment

christian-bromann commented Nov 15, 2018

andreastt commented Nov 16, 2018

andreastt commented Nov 16, 2018

christian-bromann commented Nov 16, 2018

andreastt commented Nov 16, 2018

christian-bromann commented Nov 16, 2018

andreastt commented Nov 16, 2018

jugglinmike commented Nov 16, 2018

jugglinmike commented Nov 16, 2018

andreastt commented Nov 17, 2018

andreastt left a comment

Choose a reason for hiding this comment

christian-bromann commented Nov 19, 2018

jugglinmike left a comment

Choose a reason for hiding this comment

whimboo commented Jan 28, 2019

christian-bromann commented Feb 22, 2019

whimboo commented Feb 26, 2019

jugglinmike commented Feb 26, 2019

whimboo commented Feb 26, 2019

christian-bromann commented Feb 26, 2019

whimboo commented Feb 27, 2019

gsnedders commented Feb 28, 2019

mustjab commented Feb 28, 2019

andreastt commented Feb 28, 2019

mustjab commented Feb 28, 2019

andreastt commented Feb 28, 2019

mustjab commented Feb 28, 2019

jgraham commented Mar 1, 2019

christian-bromann commented Mar 1, 2019

whimboo commented Mar 4, 2019

andreastt commented Mar 4, 2019

Hexcles commented Mar 5, 2019

christian-bromann commented Nov 15, 2018 •

edited

Loading