fix(@angular/cli): Fixed e2e task to respect --publicHost setting as baseUrl for protractor

[ReToCode]

#6173 added two new settings to the serve task. The --publicHost setting is not respected as baseUrl
for protractor in the e2e-task.

With this fix, if --publicHost is set, it will be used as baseUrl for protrator.

Not sure why 1.1.0-beta.1 is not yet on master, but this fix would be for that revision (https://github.com/angular/angular-cli/tree/v1.1.0-beta.1)

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[ReToCode]

I signed it!

[sumitarora]

@ReToCode Please rebase it against master not 1.1.0-beta

[ReToCode]

Done.

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[googlebot]

CLAs look good, thanks!

[ReToCode]

Updated to the latest version of master. Hopefully this triggers the builds again.

[ReToCode]

@sumitarora The builds seem to fail because of something unrelated to the code? Is this a known problem?
fatal: unable to access 'https://github.com/angular/angular-cli.git/': Unknown SSL protocol error in connection to github.com:443

[ReToCode]

@filipesilva Question for #4744. What is the idea behind the apiUrl as public-host in the live-reload test? My understanding is, that the public-host is set for the serve task to enable external clients to open the webpage. In our case

• Jenkins runs the webpack dev server (host=0.0.0.0 --port=portA --public-host=http://10.10.10.10:portA). So it opens on it's public ip 10.10.10.10:portA

• Selenium Grid is triggered to browse to 10.10.10.10:portA (so we need this to be the baseUrl for protractor). This is what my change is does.

The test used to tell webpack where to reach the api, but the public-host should be the url for the browser-client, not webpack. Right, or do I miss something?

[filipesilva]

@ReToCode the live-reload test in #4744 doesn't particularly care about the public-host usage, it was created and made to test a scenario prior to the current consolidated flag.

Before, it only really set the live-reload client for webpack, whereas now it sets both the live reload client and an accepted host for webpack. So that test only tries to ascertain if that option actually works by calling the live reload client and the api in a couple of different ways.

[ReToCode]

Ok. So with this change, it's no longer possible to test it this way. For now I just removed the test. Any idea to test this another way without adding a new flag for the live-reload-url?

[filipesilva]

Hm, I need to investigate this further but as a rule of thumb if you have to remove a test, you're breaking existing functionality. And that generally means the change cannot go in 1.x because it's a breaking change.

[filipesilva]

I think what's happening is:

• with your changes, --public-host is used instead of --host for protractor
• --public-host includes port
• the only thing being served on --public-host is that mock api
• app isn't served and the e2e fails

So what you're missing is to:

• use a proxy config (see proxy.ts test) to redirect /sockjs-node/info + /live-reload-count requests to the mock api
• actually serve the app on --public-host (see the public-host.ts test to see how this can be done)

[ReToCode]

Good idea. I'll try it this way one. Thanks.

[sumitarora]

As Per above ☝️

[ReToCode]

@filipesilva I tried what we discussed above. Seems like webpack does not like to proxy it's own routes. I was not able to proxy /sockjs/node. I changed the test to use the mechanism that is used in the other two cases.

As this calls /live-reload-count 2 times, it is implicitly clear that /sockjs-node/info was used by the webpage itself.

This should still result in the test case you described above

Before, it only really set the live-reload client for webpack, whereas now it sets both the live reload client and an accepted host for webpack. So that test only tries to ascertain if that option actually works by calling the live reload client and the api in a couple of different ways.

BTW, I had to change another thing in serve.ts as well. Currently the public-host parameter is without the protocol (ip:port instead of http://ip:port). Thus the url object is parsed wrong:

const clientUrl = url.parse(serveTaskOptions.publicHost);

But webpack-dev-servers public property does not work if you add the protocol to it. I think this is why it was done this way in serve.ts and then use clientUrl.href instead of clientUrl.host/hostname/port.

But this had another issue. Currently if on uses the public-host parameter (without the protocol), the /sockjs-node/info in the browser will not work. It wants to call http://ip:port/ip:port/sockjs-node/...

[filipesilva]

@ReToCode I think the test changes you made make sense, so the only thing stopping this PR from going in is the conflict. #6367 introduced some logic to allow public-host to allow hostname only, which interfere with your changes.

Thanks a lot for taking this on 👍

[ReToCode]

@filipesilva Okay I updated the PR & changed the logic in e2e to the same solution as #6367. This also fixed most of the things I talked about above.

changes were implemented

[filipesilva]

@ReToCode LGTM, and sorry it took a while to re-review.

[ReToCode]

Np. Thanks!

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}