Skip to content

Conversation

@domenic
Copy link
Member

@domenic domenic commented Aug 16, 2015

Without this change, the document's base URL is set to "/foo/bar" for the whole testing cycle. This then infects future tests, which assume that window.location is equal to document.baseURI, when they pass the former to assert_resolve_url.

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/5726

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@domenic
Copy link
Member Author

domenic commented Aug 16, 2015

I would prefer to never have the critic bot comment or email on my PRs. Is there a way I can do that apart from spam filtering?

@sideshowbarker
Copy link
Member

I would prefer to never have the critic bot comment or email on my PRs. Is there a way I can do that apart from spam filtering?

Commented on IRC; pasting in here.

about not getting how to stop the critic-bot e-mail for a wpt PR, did you try the "Exclude me, please!" button at https://critic.hoppipolla.co.uk/r/5726
maybe that won't work if you're the owner
if not, maybe you can try the "Edit owners" button and remove yourself
if none of that works, you can rebase and force-push the branch, or do something else that rewrites history for the branch, and force-push it
because critic will choke and that and stop tracking the branch
short of those I don't know any other way to disassociate yourself from a critic review, or to disassociate the critic review from a PR

Maybe @jgraham has other suggestions.

@sideshowbarker
Copy link
Member

The change in the patch here is correct but will wait for @Ms2ger to merge it, since @domenic pinged him on IRC about it.

@annevk
Copy link
Member

annevk commented Aug 17, 2015

I don't see how this is correct. Don't a lot of tests depend on the base URL defaulting?

@sideshowbarker
Copy link
Member

Don't a lot of tests depend on the base URL defaulting?

Where? There are only 5 tests in the file this change is patching.

@annevk
Copy link
Member

annevk commented Aug 17, 2015

I thought this was changing something in /url. Ignore me.

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 17, 2015

This won't work if any of the assertions fail. You should add t1.add_cleanup(function() { ... }) right after the base element is added.

Without this change, the document's base URL is set to "/foo/bar" for the whole testing cycle. This then infects future tests, which assume that window.location is equal to document.baseURI, when they pass the former to assert_resolve_url.
@domenic
Copy link
Member Author

domenic commented Aug 17, 2015

Fixed.

While we are here, there are a couple issues with "about:blank with a base element.":

  • It unnecessarily causes cross-document adoption (document.createElement("base") should be doc.createElement("base")). It seems like that should not be under test here.
  • I am 80% sure that assert_equals(doc.baseURI, document.baseURI) is incorrect. You just added a <base> element to doc with a different base URL. I haven't been able to test this in jsdom yet though since jsdom fails cross-document adoption right now :(

Ms2ger added a commit that referenced this pull request Aug 18, 2015
Fix base URL tests to not self-interfere; r=Ms2ger
@Ms2ger Ms2ger merged commit 1e70f2f into web-platform-tests:master Aug 18, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 18, 2015

Feel free to write a PR for those :)

@tobie tobie added the html label Nov 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants