web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
4.93k stars 3.07k forks source link

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

Open asankah opened 7 years ago

asankah commented 7 years ago

The tests in data-uri.htm use URLs with unescaped spaces. Consider using percent escaping so that the handling of invalid URLs don't affect the test outcome.

domenic commented 7 years ago

I think the point is to test "invalid" URL handling, among others. /cc @annevk

asankah commented 7 years ago

The invalid URL is the only URL though. Which is why I assumed that it was not intentional.

annevk commented 7 years ago

Most URLs there have unescaped spaces and as that has resulted in browser difference being uncovered it's very much intentional. (This is generally true for all tests. Tests need to cover unexpected inputs as websites will give browsers unexpected inputs. That's also why standards define how to handle such inputs.)

asankah commented 7 years ago

Are you saying that the XHR tests are where you are also testing spec compliancy of data URI parsing?

Please consider being consistent about what is being tested where. It's fine to test spec behavior WRT invalid inputs where such behavior is specified (where is invalid URL for data URIs specified?), but having that as the only test case is a bit absurd.

annevk commented 7 years ago

@asankah you cannot test this all in isolation. How would you observe a data URL response body without something like XMLHttpRequest?

RByers commented 6 years ago

@annevk, I see this test is failing with spaces being stripped on all browsers except Edge. Thank you for working to specify the behavior here, but prior to that landing is this unescaped space behavior actually specified anywhere?

If not, then perhaps it's better to change this test to not rely on spaces for now, and keep the debate about how exactly they should be handled to your new spec and tests?

annevk commented 6 years ago

@RByers I don't think it's a good idea to remove tests that demonstrate interoperability issues.

Given that browsers accept spaces in URLs and https://tools.ietf.org/html/rfc2397 doesn't say that MIME type specific processing is appropriate, it seems most logical to me that if you don't strip spaces for text/plain you also shouldn't strip them for image/png.

I'd much rather have a discussion about the underlying issue and if there's disagreement with my proposed solution for it, than about issues that arise because the underlying issue isn't solved.

Furthermore, the main reason https://github.com/whatwg/fetch/pull/579 (the new data URL standard) hasn't landed yet is because of MIME type parsing, which I'm still researching and filing numerous browser bugs on. Nobody thus far disagreed with how spaces ought to be handled. If you think we need different handling for spaces that would be the place to argue for that.

annevk commented 6 years ago

FWIW, I guess I won't object if you want to mark this test tentative for a while, but it feels a lot like busywork especially given that it's been quite hard thus far to get feedback on how to fix the relevant standards themselves. It seems to me we should all be primarily concerned with that.

asankah commented 6 years ago

@annevk : It would go a long way if you explain why this demonstrates an interop issue.

annevk commented 6 years ago

@asankah if there's an observable difference between implementations there's an interop issue.