Skip to content

Commit

Permalink
feat: set urlRoot to /__testacular__/ when proxying the root
Browse files Browse the repository at this point in the history
  • Loading branch information
@vojtajina
vojtajina committed Feb 7, 2013
1 parent d3967b5 commit 8b4fd64
Showing 1 changed file with 5 additions and 0 deletions.
5 changes: 5 additions & 0 deletions lib/config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ var normalizeConfig = function(config) {
config.urlRoot = urlRoot;
}

if (config.proxies && config.proxies.hasOwnProperty('/') && config.urlRoot === '/') {
log.warn('urlRoot set to /__testacualar__/ because of "/" proxy');
config.urlRoot = '/__testacualar__/';
}

if (config.singleRun && config.autoWatch) {
log.debug('autoWatch set to false, because of singleRun');
config.autoWatch = false;
Expand Down

14 comments on commit 8b4fd64

@bitwiseman
Copy link
Contributor

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?

@vojtajina
Copy link
Contributor Author

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 ?

@vojtajina
Copy link
Contributor Author

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.

@geddski
Copy link

@geddski geddski commented on 8b4fd64 Mar 2, 2013

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:

proxies = {
  '/angular': 'http://localhost:8000/build/angular',
  '/': 'http://localhost:8000/build/docs/'
};

Suggestions?

@vojtajina
Copy link
Contributor Author

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...

@geddski
Copy link

@geddski geddski commented on 8b4fd64 Mar 2, 2013

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.

@geddski
Copy link

@geddski geddski commented on 8b4fd64 Mar 2, 2013

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

@vojtajina
Copy link
Contributor Author

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#') or navigateTo('../index.html#...'). That would however break running the tests without Testacular (opening build/docs/docs-scenario.html). And that's not acceptable :-(

I think I'm just gonna revert the change to:

  • do not change urlRoot
  • show warning if urlRoot is being proxied

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 ?

@IgorMinar
Copy link
Contributor

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?

@vojtajina
Copy link
Contributor Author

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 open http://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).

@geddski
Copy link

Choose a reason for hiding this comment

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

FYI testacular@0.5.9 can no longer be installed once you upgrade to Node 0.10.0:

npm ERR! EEXIST, mkdir '/Users/dave/contrib/angular/node_modules/testacular/node_modules/socket.io/node_modules/socket.io-client/node_modules/uglify-js/test/unit/compress/test'
File exists: /Users/dave/contrib/angular/node_modules/testacular/node_modules/socket.io/node_modules/socket.io-client/node_modules/uglify-js/test/unit/compress/test
Move it away, and try again.

Whatever caused that was fixed in testacular@0.6.0 (or one of its deps) so I'd like to get the Angular build off testacular 0.5.9 asap.

@bitwiseman
Copy link
Contributor

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?

@vojtajina
Copy link
Contributor Author

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

@geddski
Copy link

Choose a reason for hiding this comment

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

@vojtajina perfect, thanks mate.

Please sign in to comment.