Repository navigation
Conversation
gkalpak
added
action: review
|
You can preview 51483b7 at https://pr25641-51483b7.ngbuilds.io/. |
As part of the tests run in the CircleCI `aio_monitoring` job, we need to retrieve `sitemap.xml` from the site under test. Previously, the URL used to retrieve that contained a double-slash (`//`). At some point, Firebase (which is used for hosting the site) stopped normalizing double-slashes to a single slash, causing the test to fail. This commit fixes the problem by ensuring that the constructed URLs do not contain double-slashes.
51483b7 to
267c60e
Compare
|
You can preview 267c60e at https://pr25641-267c60e.ngbuilds.io/. |
|
Where do the notifications that this monitoring task are failing go? |
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
I wonder if use of canonical-path.join and/or url.resolve would simplify a lot of this?
| } | ||
|
|
||
| export async function loadRemoteSitemapUrls(host: string) { | ||
| host = host.replace(/\/$/, ''); |
There was a problem hiding this comment.
we could just use path.join(host, 'generated/sitemap.xml') on the following line instead?
There was a problem hiding this comment.
| .then(cb); | ||
|
|
||
| await browser.get(url || browser.baseUrl); | ||
| await browser.get(url || this.baseUrl); |
There was a problem hiding this comment.
So accessing the browser.baseUrl when it ends in a forward slash errors?
There was a problem hiding this comment.
No, but I want to be consistent 😁
| await page.goTo(url); | ||
|
|
||
| const expectedUrl = prependBaseUrl(url); | ||
| const expectedUrl = stripTrailingSlash(page.baseUrl + url); |
There was a problem hiding this comment.
Could we not use path.join(page.baseUrl, url) here? That would prevent double slashes.
There was a problem hiding this comment.
Even if we used canonical-path (to avoid http:\\foo\bar), path.join() would normalize http://foo to http:/foo 😁
@kara mentioned the caretaker gets them (in some form).
B...but...this is already pretty simple 😛 |
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
OK OK - as long as it works :-)
gkalpak
added
action: merge
|
This merges cleanly to master, but not to patch. Can you create a separate PR for the patch. |
mhevery
added
target: major
|
Backported as #25820. |
…ngular#25641) As part of the tests run in the CircleCI `aio_monitoring` job, we need to retrieve `sitemap.xml` from the site under test. Previously, the URL used to retrieve that contained a double-slash (`//`). At some point, Firebase (which is used for hosting the site) stopped normalizing double-slashes to a single slash, causing the test to fail. This commit fixes the problem by ensuring that the constructed URLs do not contain double-slashes. PR Close angular#25641
…25641) (#25820) As part of the tests run in the CircleCI `aio_monitoring` job, we need to retrieve `sitemap.xml` from the site under test. Previously, the URL used to retrieve that contained a double-slash (`//`). At some point, Firebase (which is used for hosting the site) stopped normalizing double-slashes to a single slash, causing the test to fail. This commit fixes the problem by ensuring that the constructed URLs do not contain double-slashes. PR Close #25641 PR Close #25820
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
As part of the tests run in the CircleCI
aio_monitoringjob, we need to retrievesitemap.xmlfrom the site under test. Previously, the URL used to retrieve that contained a double-slash (//). At some point,Firebase (which is used for hosting the site) stopped normalizing double-slashes to a single slash, causing the test to fail.
This commit fixes the problem by ensuring that the constructed URLs do not contain double-slashes.