Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MDN examples to tests. #42

Merged
merged 1 commit into from
Jan 25, 2016
Merged

Conversation

onlywei
Copy link

@onlywei onlywei commented Jan 22, 2016

This helps ensure that passing in a URL object instead of a string to the 2nd argument of the constructor works as expected.

Related issue: whatwg/url#80

@onlywei
Copy link
Author

onlywei commented Jan 22, 2016

@domenic It looks like on master, the behavior is already OK. I added some explicit tests to ensure this.

@mightyguava I'm not sure why your situation isn't working.

@Sebmaster
Copy link
Member

Can you rebase on master? Apparently jshint broke the config file in a non-major...

@onlywei
Copy link
Author

onlywei commented Jan 22, 2016

@Sebmaster ah, I was just fixing the .jshintrc myself locally. I'll rebase.

@onlywei
Copy link
Author

onlywei commented Jan 22, 2016

@Sebmaster @domenic I just did a little more investigation into why @mightyguava is experiencing errors. It looks like the "built" lib/url.js file that we get when we npm install whatwg-url-compat does NOT pass these tests. These tests are not run against that file, and I DO get the error like this:

var URL = require('whatwg-url-compat').createURLConstructor();

var b = new URL("https://developer.mozilla.org");
var c = new URL("en-US/docs", b);

This code snippet will throw the error:

node_modules/whatwg-url-compat/lib/url.js:455
  return url.replace(/^[\u0000-\u001F\u0020]+|[\u0000-\u001F\u0020]+$/g, "");
             ^

TypeError: url.replace is not a function
    at trimControlChars (/Users/wei/development/urbancompass/src/js/node_modules/whatwg-url-compat/lib/url.js:455:14)
    at new URLStateMachine (/Users/wei/development/urbancompass/src/js/node_modules/whatwg-url-compat/lib/url.js:480:18)
    at URL.init (/Users/wei/development/urbancompass/src/js/node_modules/whatwg-url-compat/lib/url.js:1324:18)
    at new URL (/Users/wei/development/urbancompass/src/js/node_modules/whatwg-url-compat/lib/url.js:1344:10)
    at Object.<anonymous> (/Users/wei/development/urbancompass/src/js/url-test.js:6:9)
    at Module._compile (module.js:435:26)
    at Object.Module._extensions..js (module.js:442:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:311:12)
    at Function.Module.runMain (module.js:467:10)

Opening an issue also about this, I don't think I can fix this on my own.

@Sebmaster
Copy link
Member

The packages on npm haven't been updated to the new rework yet since I'll still have to review the url PR on jsdom to see how they interface with each other.

The tests are executed against the built file (the source file actually contains an undefined function, so wouldn't actually work).

describe("MDN examples", () => {
it("Checking all examples on MDN pass", () => {
const a = new URL("/", "https://developer.mozilla.org");
assert.strictEqual(a.href, "https://developer.mozilla.org/", "a");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please get rid of these "a", "b", ... "j" strings.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask why? They correspond to the MDN examples and help determine which of the assert statements are actually failing. Without them, it would be difficult to see since many of the expected strings are identical.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't help me as a developer at all. They cause the assertion message to be "a" instead of "asdfasdf did not strict equal https://developer.mozilla.org/"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can tell which assert is failing from the line number.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'll remove them.

const h = new URL("/en-US/docs", a);
assert.strictEqual(h.href, "https://developer.mozilla.org/en-US/docs");

// Raises a SYNTAX ERROR exception as "/en-US/docs" is not valid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is incorrect (maybe MDN is incorrect). It should be a TypeError.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible that MDN is incorrect. I'll just remove the comment.

This helps ensure that passing in a URL object
instead of a string to the 2nd argument of the
constructor works as expected.
@onlywei
Copy link
Author

onlywei commented Jan 22, 2016

Hmm, npm run build and npm test and npm lint all seem to pass locally, not sure why Travis is failing.

@onlywei
Copy link
Author

onlywei commented Jan 22, 2016

Hmm, Travis seems to have been stuck on nvm install stable at 4.8% for the past 2 hours.

@Sebmaster
Copy link
Member

Yeah, seems like it has a bit of a problem, I'll get to reviewing all changes tomorrow at the earliest anyways though so maybe it'll be fixed by then.

@Sebmaster Sebmaster merged commit 181824e into jsdom:master Jan 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants