data: URL tests #6890

annevk · 2017-08-15T10:43:45Z

annevk · 2017-08-15T10:44:27Z

This initial commit contains tests for base64 (and makes sure they're shared with atob() so any new tests will cover both).

data: URL-specific tests to follow.

annevk · 2017-08-15T10:51:02Z

Bugs:

foolip · 2017-08-30T14:54:14Z

fetch/data-urls/README.md

@@ -0,0 +1,11 @@
+== data: URLs ==
+
+`resources/data-urls.json` contains `data:` URL tests. The tests are encoded as a JSON array. Each value in the array is an array of two are three values. The first value describes the input, the second value describes the expected MIME type, null if the input is expected to fail somehow, or the empty string if the expected value is `text/plain;charset=US-ASCII`. The third value, if present, describes the expected body as an array of integers representing bytes.


two or three?

ghost · 2017-09-18T13:18:16Z

Build PASSED

Started: 2018-01-30 18:19:36
Finished: 2018-01-30 18:29:28

View more information about this build on:

foolip · 2017-09-27T14:41:03Z

fetch/data-urls/resources/data-urls.json

+  ["data:;charset=\"x\",X",
+   "text/plain;charset=\"x\"",
+   [88]],
+  ["data:;CHARSET=\"X\",X",


I guess this would be the place to test the questions in whatwg/fetch#579 (comment)? Or are such tests already here?

Yeah, a little above this one. Line 163, 166, and various tests around those too.

annevk · 2017-11-14T17:47:56Z

fetch/data-urls/resources/data-urls.json

+   "text/plain;charset=x",
+   [88]],
+  ["data:;charset= x,X",
+   "text/plain;charset=x",


This pass condition should change to preserve the space before the lowercase x.

yutakahirano · 2017-11-15T08:34:58Z

fetch/data-urls/resources/data-urls.json

+  ["data:;charset= x,X",
+   "text/plain;charset=x",
+   [88]],
+  ["data:;charset=,X",


An empty string is not a valid token and hence this will result in text/plain;charset=US-ASCII, I guess.

It'll result in text/plain. We only use the fallback if MIME type parsing returns missing or failure which it won't for text/plain;charset=.

Tests for whatwg/fetch#579.

annevk · 2018-01-05T14:56:03Z

@domenic if you have ideas for more comma tests btw let me know.

sideshowbarker · 2018-01-07T01:03:09Z

w3c-test:mirror

annevk · 2018-01-07T07:31:53Z

@domenic I think I addressed all your feedback now.

domenic · 2018-01-08T23:15:13Z

fetch/data-urls/resources/data-urls.json

+   [87, 65]],
+  ["data:x;base64;x,WA",
+   "",
+   [88]],


Per my tests this should be [87, 65] like the above (doesn't end in ;base64).

domenic · 2018-01-08T23:15:55Z

fetch/data-urls/processing.any.js

+    const expectedBody = expectedMimeType !== null ? tests[i][2] : null;
+    promise_test(t => {
+      if(expectedMimeType === "") {
+        expectedMimeType = "text/plain;charset=US-ASCII";


It's still really, really confusing looking at the test files and seeing empty string as the expected MIME type. It'd be quite nice if this was fixed.

If nothing else, maybe replace it with "__default__" or something, with a comment at the top explaining the substitution.

foolip · 2018-01-19T22:10:38Z

fetch/data-urls/resources/data-urls.json

+  ["data:%00,%FF",
+   "",
+   [255]],
+  ["data:text/html  ,X",


Given whatwg/fetch#579 (comment) I think this would be a good place to have a negative test for the odd thing that Safari now has, for example with "data:text / html,X", and asserting that it's treated as an invalid MIME type. Should I just push that?

domenic · 2018-01-24T04:07:00Z

After updating to whatwg/fetch@cedd3f3 I get the following failure with the tests, which don't seem updated yet:

data:; base64,WA expected "text/plain", but implementation produced "text/plain;charset=US-ASCII"

Probably more tests should be added for those spec changes, as just that one seems pretty small.

foolip

I added the promised test, have reviewed the recent additions, and can't think of anything more. Great stuff!

foolip · 2018-01-24T15:55:52Z

Note that the "data:; base64,WA" @domenic found was fixed in 8d4335f

I won't block on the format, but I still am unhappy.

domenic · 2018-01-25T06:12:53Z

These tests appear to all match the spec, as tested by a JS implementation of it.

I still am pretty frustrated that my feedback on using the actual MIME type instead of a magic empty string is not being taken into account in the data file; I find this kind of mix of useful, readable, expected results with magic sentinels that cause a translation layer to be invoked frustrating. I think it would be nice to listen to your test consumers, and optimize for reading and consumption (which happen many times) more than writing (which happens once).

But I won't block merging based on that. I just want to issue a final plea.

foolip · 2018-01-25T06:15:18Z

If "__default__" is acceptable to @domenic, then that seems like an improvement to me.

domenic · 2018-01-25T06:16:43Z

It's not as good as the actual result, as it still requires a translation layer, but at least it's less likely to be interpreted as a literal empty string.

domenic · 2018-01-25T06:21:42Z

Some additional fun tests might be for things that Unicode-compare equal to base64, but don't ASCII-compare equal to it. I'm not sure what those would be exactly, but if I'd written /(.*); *base64$/i instead of /(.*); *[Bb][Aa][Ss][Ee]64$/, I would have gotten this wrong.

foolip · 2018-01-25T06:30:40Z

Is having null as a sentinel also a problem, or just changing one string to another string? Are you doing things with the JSON file other than run these tests?

Just expanding to the real value would be fine by me I guess, when there are failures the values will already have been expanded so maybe it'll be easier to find in the JSON file then.

domenic · 2018-01-25T06:34:35Z

A sentinel for failure (viz. null) makes sense; you actually need to have your test harness behave differently.

A sentinel that is just replacing one string with another means you can't do string comparison, or read the expected results in the JSON file, but instead have to transform the test JSON file into an actually-expected-results.json before doing the comparisons.

Are you doing things with the JSON file other than run these tests?

Well, I'm trying to read it to figure out what the results should be, while debugging test failures. Having to mentally translate empty string to mean... what was the exact string? I guess I'll go look at README.md... is a definite burden.

foolip · 2018-01-25T08:58:10Z

I see, that sounds like a real burden worth avoiding. I originally somewhat uncharitably took the argument to be a principled one about minimizing computation in tests, even when it does make things clearer, as I think it did for me as a reviewer.

annevk · 2018-01-25T13:38:15Z

@domenic Unicode gets percent-encoded, no? I'm happy to expand the empty string btw, once everything else is in order.

domenic · 2018-01-30T16:48:38Z

Confirmed the new EOF cases close that coverage gap.

I guess there is no way to trigger Unicode-but-not-ASCII case insensitive equality, you're right.

domenic

Thanks for writing out the MIME type :)

Unfortunately RFC 2397 has some ambiguities and implementations never really followed it in detail. Tests: web-platform-tests/wpt#6890. Fixes #234.

SimonSapin · 2018-02-02T11:26:49Z

fetch/data-urls/resources/data-urls.json

+   "text/plain;charset=US-ASCII",
+   [88]],
+  ["data://test:test/,X",
+   null],


Why does this test case fail? Is it the URL parser that returns an error?

This URL loads an X document in Firefox and Chromium. (It doesn’t in Edge, but Edge appears to reject any data: URL with a MIME type parsing error.)

Unfortunately RFC 2397 has some ambiguities and implementations never really followed it in detail. Tests: web-platform-tests/wpt#6890. Fixes #234.

wpt-pr-bot added fetch html labels Aug 15, 2017

wpt-pr-bot requested review from jdm, jgraham, mnot, youennf, zcorpan and zqzhang August 15, 2017 10:43

annevk mentioned this pull request Aug 21, 2017

base64 w3c/push-api#280

Open

foolip reviewed Sep 4, 2017

View reviewed changes

annevk force-pushed the annevk/data-urls branch from 7fd5ded to bc79c1a Compare September 18, 2017 13:18

wpt-pr-bot requested a review from yutakahirano September 18, 2017 13:18

web-platform-tests deleted a comment Sep 18, 2017

web-platform-tests deleted a comment Sep 19, 2017

foolip reviewed Sep 27, 2017

View reviewed changes

RByers mentioned this pull request Oct 27, 2017

XMLHttpRequest/data-uri.htm uses invalid URLs #7374

Open

annevk commented Nov 14, 2017

View reviewed changes

annevk mentioned this pull request Nov 14, 2017

Revamp MIME type section whatwg/mimesniff#36

Merged

3 tasks

yutakahirano reviewed Nov 15, 2017

View reviewed changes

annevk mentioned this pull request Nov 24, 2017

Sort out MIME type tests whatwg/mimesniff#42

Closed

4 tasks

annevk force-pushed the annevk/data-urls branch from 55e3b06 to 6509b9c Compare November 25, 2017 09:28

annevk added 5 commits November 25, 2017 11:21

data: URL tests

2ab6cbd

Tests for whatwg/fetch#579.

Add data URL tests

5b1e170

more data: URL ;base64 tests

0c167bf

base64: ensure correct character set is used, test example from spec

79defd3

even more base64 data: URL tests

7764395

address all feedback by Domenic

8d685a5

domenic previously requested changes Jan 8, 2018

View reviewed changes

final bug (fingers crossed)

17f202d

foolip reviewed Jan 19, 2018

View reviewed changes

annevk and others added 2 commits January 24, 2018 14:36

add some more tests

8d4335f

test "data:text / html,X" (the Safari oddity)

7ffe10e

foolip approved these changes Jan 24, 2018

View reviewed changes

add some EOF cases per feedback

fdf9418

flatten

7638432

domenic approved these changes Jan 30, 2018

View reviewed changes

annevk merged commit 7eec2bf into master Jan 31, 2018

annevk deleted the annevk/data-urls branch January 31, 2018 08:57

annevk added a commit to whatwg/fetch that referenced this pull request Jan 31, 2018

Define data: URL processing

36ef3c8

Unfortunately RFC 2397 has some ambiguities and implementations never really followed it in detail. Tests: web-platform-tests/wpt#6890. Fixes #234.

SimonSapin reviewed Feb 2, 2018

View reviewed changes

rsumner31 added a commit to rsumner31/fetch1 that referenced this pull request Mar 15, 2018

Define data: URL processing

6c6ef29

Unfortunately RFC 2397 has some ambiguities and implementations never really followed it in detail. Tests: web-platform-tests/wpt#6890. Fixes #234.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data: URL tests #6890

data: URL tests #6890

annevk commented Aug 15, 2017 •

edited by wpt-pr-bot

Loading

annevk commented Aug 15, 2017

annevk commented Aug 15, 2017

foolip Aug 30, 2017

ghost commented Sep 18, 2017 •

edited by ghost

Loading

foolip Sep 27, 2017

annevk Sep 27, 2017

annevk Nov 14, 2017

yutakahirano Nov 15, 2017

annevk Nov 25, 2017

annevk commented Jan 5, 2018

sideshowbarker commented Jan 7, 2018

annevk commented Jan 7, 2018

domenic Jan 8, 2018

domenic Jan 8, 2018

foolip Jan 19, 2018

annevk Jan 20, 2018

foolip Jan 24, 2018

domenic commented Jan 24, 2018

foolip left a comment

foolip commented Jan 24, 2018

domenic commented Jan 25, 2018

foolip commented Jan 25, 2018 •

edited

Loading

domenic commented Jan 25, 2018

domenic commented Jan 25, 2018 •

edited

Loading

foolip commented Jan 25, 2018

domenic commented Jan 25, 2018

foolip commented Jan 25, 2018

annevk commented Jan 25, 2018

domenic commented Jan 30, 2018

domenic left a comment

SimonSapin Feb 2, 2018

SimonSapin Feb 2, 2018

annevk Feb 2, 2018

		@@ -0,0 +1,11 @@
		== data: URLs ==

		`resources/data-urls.json` contains `data:` URL tests. The tests are encoded as a JSON array. Each value in the array is an array of two are three values. The first value describes the input, the second value describes the expected MIME type, null if the input is expected to fail somehow, or the empty string if the expected value is `text/plain;charset=US-ASCII`. The third value, if present, describes the expected body as an array of integers representing bytes.

data: URL tests #6890

data: URL tests #6890

Conversation

annevk commented Aug 15, 2017 • edited by wpt-pr-bot Loading

annevk commented Aug 15, 2017

annevk commented Aug 15, 2017

Choose a reason for hiding this comment

ghost commented Sep 18, 2017 • edited by ghost Loading

Build PASSED

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

annevk commented Jan 5, 2018

sideshowbarker commented Jan 7, 2018

annevk commented Jan 7, 2018

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

domenic commented Jan 24, 2018

foolip left a comment

Choose a reason for hiding this comment

foolip commented Jan 24, 2018

domenic commented Jan 25, 2018

foolip commented Jan 25, 2018 • edited Loading

domenic commented Jan 25, 2018

domenic commented Jan 25, 2018 • edited Loading

foolip commented Jan 25, 2018

domenic commented Jan 25, 2018

foolip commented Jan 25, 2018

annevk commented Jan 25, 2018

domenic commented Jan 30, 2018

domenic 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

annevk commented Aug 15, 2017 •

edited by wpt-pr-bot

Loading

ghost commented Sep 18, 2017 •

edited by ghost

Loading

foolip commented Jan 25, 2018 •

edited

Loading

domenic commented Jan 25, 2018 •

edited

Loading