deprecate / metal-uri

Class for parsing and formatting URIs.
Other
3 stars 7 forks source link

Unit test for parseFromAnchor function is dodgy #13

Closed vbence86 closed 7 years ago

vbence86 commented 7 years ago
Declaimer

This module has been linked to Travis CI in travis branch (https://github.com/metal/metal-uri/tree/travis), but the test apparently fails against certain Saucelabs browsers.

Travis Output

https://travis-ci.org/metal/metal-uri/builds/258612453

Recurring issues

While looking into this issue we came across that the function responsable for URL validation is a bit dodgy and fails in its purpose. It creates an <a> instance and sets its href attribute against the given URL that is believed to be invalid. The specified browsers taking part in the test nevertheless automatically generate a valid URL out of this invalid one by invoking a transparent native code as soon as the <a> instance's href attribute is altered.

Proposal

Please double check whether or not the existence of this test provides any value to the building process.

The dodgy code snippet copied from parseFromAnchor.js

    var link = document.createElement('a');
    link.href = opt_uri;

    if(link.protocol === ':' || !/:/.test(link.href)) {
        throw new TypeError(`${opt_uri} is not a valid URL`);
    }
jbalsas commented 7 years ago

Hey @vbence86, please see https://github.com/metal/metal-uri/issues/11 for the rationale behind the TypeError exception and https://github.com/metal/metal-uri/pull/12 for the PR that went in.

Maybe we missed something?

vbence86 commented 7 years ago

@jbalsas Thanks for shading some light on this, the initial goal of the test is now crystal clear. According to the WHATWG living standard of URI the relevant definition is the following:

If port is greater than 2^16 − 1, validation error, return failure.

I suggest to change the description of the test case at first. I've created a PR for that - https://github.com/metal/metal-uri/pull/16

The reason why the tests are failing is that various browsers do not parse the given URL with regards to this expectation and the link.protocol doesn't equal to : character.

vbence86 commented 7 years ago

To follow this up, I've created a Pull Request that addresses the mentioned issue. https://github.com/metal/metal-uri/pull/17

robframpton commented 7 years ago

Hey @vbence86

I'm assuming #18 closes this issue?

vbence86 commented 7 years ago

@Robert-Frampton Indeed, I merely forgot to update that issue after #18 had been merged.