Skip to content

Tests: fix cross-domain detection test for non-default port #1954

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

Closed
wants to merge 1 commit into from

Conversation

victor-homyakov
Copy link
Contributor

Tests: fix cross-domain detection test for non-default port

@dmethvin
Copy link
Member

Can you add a unit test?

@victor-homyakov
Copy link
Contributor Author

Add a test that another test passes?

@victor-homyakov
Copy link
Contributor Author

This PR fixes an AJAX test which was properly passing only with default (80) port (when location.host === location.hostname) and failing when running on non-default (thus not omitted from host) port.

@dmethvin
Copy link
Member

OK, so that makes the unit test a bit hard. We'd need to make a request to a non-default port and we don't have a test server running there.

N.B. We'll need an issue describing the problem.

@victor-homyakov
Copy link
Contributor Author

No. Suppose you have your test server installed on 8080 port. In this case test will fail because URL string will look like http://localhost:8080:8080 because of host + port wrongly used instead of hostname + port.

@dmethvin
Copy link
Member

Right, so if we want to test both port 80 and port 8080 to ensure that both work, i was thinking we'd need two servers. But really we can just make the request, let it massage the URL, inspect the result, and cancel it before the request. Does that make sense? We need a unit test to do that.

@victor-homyakov
Copy link
Contributor Author

One line of code is worth thousand words. Just show how this test will look like and what it will actually check. I think we cannot test cross-origin detection for 8080 port without test page actually running on that port.

@victor-homyakov
Copy link
Contributor Author

Ping?

@dmethvin
Copy link
Member

dmethvin commented Jan 7, 2015

Sorry, I lost track of this. Looking at it with fresh eyes, I now realize what you are saying! The change is only in the unit test so I'm good with landing it as-is.

@dmethvin dmethvin closed this in 83b038f Jan 7, 2015
@victor-homyakov victor-homyakov deleted the patch-2 branch August 20, 2015 09:59
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants