-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Handle all valid URL path characters for rootURL/baseURL in tests-server #5909
Conversation
@@ -41,7 +41,8 @@ TestsServerAddon.prototype.serverMiddleware = function(config) { | |||
var filePath = path.join(results.directory, assetPath); | |||
|
|||
if (!existsSync(filePath) || !fs.statSync(filePath).isFile()) { | |||
var newURL = baseURL + '/tests/index.html'; | |||
// N.B., `baseURL` will end with a slash as it went through `cleanBaseURL` | |||
var newURL = baseURL + 'tests/index.html'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appeared to be an existing bug. Without this change I got a double /
, making my assertions fail: /braden/+//tests/index.html
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, no need for that code comment though IMHO
@rwjblue r? |
The AppVeyor problem is related to phantomjs-prebuilt not being installed (for the Environment: nodejs_version=4.2). Does not seem related to these changes. retest this
|
This works for me. The appveyor build probably needs a re-run to get around a temporary network problem?
|
kicking appveyor now |
900b15a
to
d8607dd
Compare
Pushed a change to a comment to make build run again. |
Looks good in general 👍 Not sure what's up with CI at the moment... |
@les2 could you rebase on the current master branch? hopefully that should solve the CI issues... |
@Turbo87 rebasing ... |
Unit test demonstrating bug caused by `+` in baseURL or rootURL.
The previous implementation used a regular expression for matching the beginning of the request path against the baseURL + 'tests' but did not escape the baseURL. Because RFC 3986 allows some characters that are not valid in regexes, this caused problems when using them for the baseURL. Closes ember-cli#5876
d8607dd
to
17465cd
Compare
@Turbo87 @stefanpenner CI passes now |
📌 Commit 17465cd has been approved by |
⚡ Test exempted - status |
Handle all valid URL path characters for rootURL/baseURL in tests-server The previous implementation used a regular expression for matching the beginning of the request path against the baseURL + 'tests' but did not escape the baseURL. Because RFC 3986 allows some characters that are not valid in regexes, this caused problems when using them for the baseURL. Closes #5876
@stefanpenner @nathanhammond I'd like to pull this into the |
"tests-server" test cleanup This PR cleans up the "tests-server" unit test now that #5909 is merged.
Late to seeing this. It's in beta now! 😛 |
The previous implementation used a regular expression for matching the
beginning of the request path against the baseURL + 'tests' but did
not escape the baseURL. Because RFC 3986 allows some characters that
are not valid in regexes, this caused problems when using them for
the baseURL.
Closes #5876