-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat: set urlRoot to /__testacular__/ when proxying the root
- Loading branch information
Showing
1 changed file
with
5 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8b4fd64
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.
My config proxies root and doesn't set urlRoot (default is '/'), so this would effect me. What is the purpose of this change?
8b4fd64
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.
Well, it does effect you.
It's becaused many people had this issue when they set proxy for the root, but then it does not proxy the root (or other paths that are already taken by testacular). So in theory, you shoul always set urlRoot, if you proxy the root. Does it cause any issue for you ?
8b4fd64
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.
However, if you rely on Testacular to capture the browsers, you don't have to worry about it, it will capture it on the correct urlRoot.
8b4fd64
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.
@vojtajina This breaks Angular's end-to-end test. We're proxying root for the docs:
Suggestions?
8b4fd64
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.
What's the error ? It only changes testacular to run in a subpath, to avoid conflicts with your proxy...
8b4fd64
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.
It can't find the html files so all the tests fail:
404 Not Found: /build/docs/__testacualar__/index-nocache.html
The correct path is
/build/docs/index-nocache.html
. I've tried setting the urlRoot to a few different things but no luck yet.8b4fd64
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.
Here's the config file: https://github.com/angular/angular.js/blob/master/testacular-e2e.conf.js
8b4fd64
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.
I see. You could fix that either by
navigateTo('/index.html#')
ornavigateTo('../index.html#...')
. That would however break running the tests without Testacular (openingbuild/docs/docs-scenario.html
). And that's not acceptable :-(I think I'm just gonna revert the change to:
That means, Angular's docs scenario tests will produce a Testacular warning.
We could even remove the warning completely, but the reason why I did this was that many people run into the issue where they were hitting Testacular rather their proxied app. (Testacular has to have precedence, so if you are proxying
/
, you can't use/
, neither/debug.html
,/base/...
,/absolute/...
,/socket.io/...
- these won't be proxied)@IgorMinar any thoughts on this ?
8b4fd64
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.
can't we set
base[href]
to get the angular tests to work properly?with this change in, what happens when I navigate to
http://localhost:9876/
? Do I capture the browser or do I get proxied into the app?btw is the typo in
__testacualar__
intentional?8b4fd64
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.
Once using urlRoot =
/__testacular__/
, you need to openhttp://localhost:9876/__testacular__/
to capture the browser.The typo was not intentional, thanks.
I think setting
base[href]
(inside context.html and debug.html) would solve this particular issue. I remember we used to set the base[href] and it was causing other issues (https://github.com/testacular/testacular/commit/bea89dc52ca20b9fda123bfccc2c30191c1f1949).8b4fd64
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.
FYI [email protected] can no longer be installed once you upgrade to Node 0.10.0:
Whatever caused that was fixed in [email protected] (or one of its deps) so I'd like to get the Angular build off testacular 0.5.9 asap.
8b4fd64
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.
I think you meant to put this in a separate issue. Yes?
8b4fd64
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.
@GEDDesign I just created an issue for it #400, thanks for heads up, I haven't noticed 0.10 is out.
Ad. the original issue - after discussion with @IgorMinar I'm reverting this change, only keeping the warning, without changing the
urlRoot
(see 8c138b5). That means, Angular's e2e tests will produce warning, but it will work.Long term, we will remove the warning and do #399.
I will merge some other fixes and release 0.6.1
8b4fd64
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.
@vojtajina perfect, thanks mate.