aio: add monitoring and fix a couple of issues #22483

gkalpak · 2018-02-27T23:48:24Z

Fix: Wait for the app to stabilize before registering the SW.
Wait for the app to stabilize, before registering the SW and starting to check for SW updates. This avoids setting up a long timeout, which would prevent the app from stabilizing and thus cause issues with Protractor.
Fix: Fix SW routing RegExp to allow redirecting /api/animate URLs.
Prevent the SW from routing URLs starting with /api/animate to index.html. Instead, pass them through to the server, where they are correctly redirected.
CI: Add monitoring for angular.io.
Configure a periodic job to be run on CircleCI, performing several checks against the actual apps deployed to production (https://angular.io) and staging (https://next.angular.io).

Fixes #21942.

mary-poppins · 2018-02-27T23:56:00Z

You can preview 676a999 at https://pr22483-676a999.ngbuilds.io/.

petebacondarwin

A few minor nits that you can choose to ignore but otherwise looks good.
I like what you did with redirect and sitemap testing !

petebacondarwin · 2018-02-28T10:51:48Z

aio/src/app/sw-updates/sw-updates.service.ts

-        .debounceTime(this.checkInterval)
-        .startWith(null)
+  constructor(appRef: ApplicationRef, private logger: Logger, private sw: NgServiceWorker) {
+    const appIsStable$ = appRef.isStable.filter(v => v).first();


this can be simplified to:

appRef.isStable.first(v => v);

See http://reactivex.io/documentation/operators/first.html

I had no idea about this 💯

petebacondarwin · 2018-02-28T10:59:27Z

aio/src/app/sw-updates/sw-updates.service.ts

@@ -60,7 +62,7 @@ export class SwUpdatesService implements OnDestroy {
    this.sw.checkForUpdate()
        // Temp workaround for https://github.com/angular/mobile-toolkit/pull/137.
        // TODO (gkalpak): Remove once #137 is fixed.
-        .concat(Observable.of(false)).take(1)
+        .concat(of(false)).first()


As I understand it we want to emit a false if the stream closes without emitting anything, right?
If so, can this be simplified to the following?

.defaultIfEmpty(false)

I didn't know about this one either. Nice!
(I see someone knows their RxJS 😃)

petebacondarwin · 2018-02-28T11:12:33Z

aio/src/main.ts

@@ -11,7 +13,7 @@ if (environment.production) {
 platformBrowserDynamic().bootstrapModule(AppModule).then(ref => {
  if (environment.production && 'serviceWorker' in (navigator as any)) {
    const appRef: ApplicationRef = ref.injector.get(ApplicationRef);
-    appRef.isStable.first().subscribe(() => {
+    appRef.isStable.filter(v => v).first().subscribe(() => {


Can be simplified to:

appRef.isStable.first(v => v).subscribe(() => {

petebacondarwin · 2018-02-28T11:19:17Z

aio/tests/deployment-config/unit/testFirebaseRedirection.spec.ts

@@ -1,8 +1,8 @@
-import { getRedirector, loadLegacyUrls, loadRedirects, loadSitemapUrls } from './helpers';
+import { getRedirector, loadLegacyUrls, loadRedirects, loadLocalSitemapUrls } from '../shared/helpers';


AFAICT loadLocalSitemapUrls does not exist...

I see it was added in a later commit :-O

petebacondarwin · 2018-02-28T11:21:18Z

aio/tests/deployment-config/shared/URLS_TO_REDIRECT.txt

@@ -180,3 +182,8 @@
 /news.html	https://blog.angular.io/
 /testing	/guide/testing
 /testing/first-app-tests.html	/guide/testing
+
+# These URLs are deliberately put at the end in order to ensure that the ServiceWorker will have
+# been installed. This allows verifying that the SW will indeed let them pass through to the server.


Are you relying upon the SW loading during processing of these tests? Why else would it matter that they are at the end of the file?

Yes, I rely on the SW being loaded.

Essentially, if you put these URLs at the top of the file (where they belong alphabetically 😁), they are navigatd to before the SW has been installed. As a result, they pass through to the server, where they are redirected correctly.

Once the SW has been activated and handles navigation, these URLs will only pass through to the server if they are not matched by the routing RegExp in ngsw-manifest.json.

In other words:
Having the URLs at the top would allow the tests to pass even with the current (broken) RegExp on master.
Having the URLs at the end, makes the tests fail on master (and pass with the fix in this PR).

What? Surely not?
Is there not some other mechanism to ensure that the SW has been activated before running these tests?
Perhaps a dummy request in the protractor spec or something?

I just realized my comment might be confusing, because I am referring to the e2e tests added in the next commit 😁
The jasmine tests (run with yarn deployment-config-test) will correctly fail/pass regardless where you put the URLs 😁

I understand but I still think it is rather flakey to rely upon the order of the test data in this way.

I know 😞
(TBH, I don't think it matters, because we already assert that these URLs are handled correctly by the SW in depoyment-config-test, but I will look into it.)

I didn't find a reliable way to ensure that the ServiceWorker has been activated, but I also think that we mainly want to check the Firebase server (since we are pretty confident about the SW routing RegExp based on the tests in deployment-config-test).

So, I changed the code to instead unregister the SW before each test 😁

petebacondarwin · 2018-02-28T11:27:15Z

aio/package.json

    "build-ie-polyfills": "node node_modules/webpack/bin/webpack.js -p src/ie-polyfills.js src/generated/ie-polyfills.min.js",
+    "update-webdriver": "webdriver-manager update --standalone false --gecko false $CHROMEDRIVER_VERSION_ARG",


AFAICT renaming ~update-webdriver to update-webdriver is the only real change in this file, right? Or am I missing something?

Pretty much 😇

petebacondarwin · 2018-02-28T11:30:24Z

aio/tests/deployment-config/shared/helpers.ts

@@ -34,6 +28,27 @@ export function loadLegacyUrls() {
      .map(line => line.split('\t'));            // Split lines into URL pairs.
 }

+export function loadLocalSitemapUrls() {


Ahah! I see that the non-existent loadLocalSitemapUrls was a rebase mismatch.

mary-poppins · 2018-02-28T15:52:21Z

You can preview 7873194 at https://pr22483-7873194.ngbuilds.io/.

mary-poppins · 2018-02-28T16:04:56Z

You can preview 9148010 at https://pr22483-9148010.ngbuilds.io/.

gkalpak · 2018-02-28T19:27:32Z

@petebacondarwin, I added a bunch of fixup commits. (I didn't touch the non-fixup commits other than rebasing on master.) PTAL

mary-poppins · 2018-02-28T19:54:04Z

You can preview 52ceab0 at https://pr22483-52ceab0.ngbuilds.io/.

IgorMinar

this looks great otherwise. I just wonder if testing all the urls is not going to take too long and be too flaky. Have you considered that?

IgorMinar · 2018-03-02T23:22:50Z

aio/tests/deployment-config/e2e/testDeployedRedirection.e2e-spec.ts

+        const expectedUrl = browser.baseUrl + url;
+        const actualUrl = (await browser.getCurrentUrl()).replace(/\?.*$/, '');
+
+        expect(actualUrl).toBe(expectedUrl);


shouldn't we also verify that we didn't render an error or 404 page instead?

if you could add an assertion for the content of the doc-viewer DOM element I'd trust this test more.

Actually, while this would be nice, it would require waiting for the app to stabilize, which in turn would dramatically increase the total time (~5x).

In that case we should add these more expensive assertions just for a smaller sample of urls. maybe in a separate pr?

I created #23063 to track that. Let's land this and see how it works for a few days first.

gkalpak · 2018-03-05T13:36:40Z

Pushed another fixup commit.

Long running times are indeed a concern. After I realized I was doing an extra request per sitemap/legacy URL, the time for those tests was brought down to ~7.5mins (from ~17mins), which brings the overall time down to ~11mins per app URL. So, it would be ~22mins (plus setup time) for our two URLs (angular.io and next.angular.io), which sounds reasonable for this kind of job.

If we want to reduce these times further, we could consider probing some of the URLs only. (But I would avoid randomness to retain reproducibility. Maybe we can test a subset of the URLs based on the current date or something.)

I've not seen any flakeyness, when running this locally, so I think it is reasonable to not expect any more flakeyness that normally have on CI jobs 😁
But, only time will tell for sure 🤔

mary-poppins · 2018-03-05T13:44:04Z

You can preview c2bcfe5 at https://pr22483-c2bcfe5.ngbuilds.io/.

alxhub · 2018-03-29T16:07:14Z

This doesn't apply cleanly to the patch branch (conflicts with Rx pipeable operators update). Please split it into two PRs :) Thanks!

This commit also waits for the app to stabilize, before starting to check for ServiceWorker updates. This avoids setting up a long timeout, which would prevent the app from stabilizing and thus cause issues with Protractor.

This commit prepares the ground for adding different types of tests.

…mate` URLs

This commit configures a periodic job to be run on CircleCI, performing several checks against the actual apps deployed to production (https://angular.io) and staging (https://next.angular.io). Fixes angular#21942

gkalpak · 2018-03-30T18:44:10Z

OK. Let's merge this into master and I'll submit another PR for 5.2.x.
(EDIT: #23093 for 5.2.x)

This commit prepares the ground for adding different types of tests. PR Close #22483

…RLs (#22483) PR Close #22483

This commit configures a periodic job to be run on CircleCI, performing several checks against the actual apps deployed to production (https://angular.io) and staging (https://next.angular.io). Fixes #21942 PR Close #22483

angular-automatic-lock-bot · 2019-09-13T11:52:01Z

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

googlebot added the cla: yes label Feb 27, 2018

gkalpak added type: bug/fix area: build & ci Related the build and CI infrastructure of the project action: review The PR is still awaiting reviews from at least one requested reviewer comp: aio target: patch This PR is targeted for the next patch release labels Feb 27, 2018

petebacondarwin approved these changes Feb 28, 2018

View reviewed changes

petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 28, 2018

gkalpak force-pushed the ci-aio-monitoring branch from 676a999 to 7873194 Compare February 28, 2018 15:41

gkalpak force-pushed the ci-aio-monitoring branch from 7873194 to 9148010 Compare February 28, 2018 15:57

gkalpak force-pushed the ci-aio-monitoring branch 2 times, most recently from c3a91a8 to 52ceab0 Compare February 28, 2018 19:23

petebacondarwin approved these changes Feb 28, 2018

View reviewed changes

gkalpak requested a review from IgorMinar March 1, 2018 12:24

gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 1, 2018

IgorMinar approved these changes Mar 2, 2018

View reviewed changes

IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 2, 2018

gkalpak mentioned this pull request Mar 4, 2018

aio: send crash reports to Google Analytics #21943

Closed

gkalpak force-pushed the ci-aio-monitoring branch 2 times, most recently from 039e44c to c2bcfe5 Compare March 5, 2018 13:36

IgorMinar approved these changes Mar 29, 2018

View reviewed changes

gkalpak mentioned this pull request Mar 29, 2018

aio: verify no 404 is served for sitemap URLs #23063

Closed

alxhub added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Mar 29, 2018

gkalpak added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Mar 30, 2018

gkalpak added 10 commits March 30, 2018 17:49

fixup! fix(aio): wait for the app to stabilize before registering the SW

cc5ee5f

refactor(aio): move deployment config tests and helpers around

cdc2e09

This commit prepares the ground for adding different types of tests.

fixup! refactor(aio): move deployment config tests and helpers around

a353b4b

fix(aio): fix SW routing RegExp to allow redirecting /api/animate URLs

2fc4428

fixup! fix(aio): fix SW routing RegExp to allow redirecting `/api/ani…

873284c

…mate` URLs

ci(aio): add monitoring for angular.io

014ce82

This commit configures a periodic job to be run on CircleCI, performing several checks against the actual apps deployed to production (https://angular.io) and staging (https://next.angular.io). Fixes angular#21942

fixup! ci(aio): add monitoring for angular.io

dc97fd6

fixup! ci(aio): add monitoring for angular.io

b403663

fixup! fix(aio): wait for the app to stabilize before registering the SW

a3000e2

gkalpak force-pushed the ci-aio-monitoring branch from f35dec6 to a3000e2 Compare March 30, 2018 14:56

gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 30, 2018

gkalpak mentioned this pull request Mar 30, 2018

aio: add monitoring and fix a couple of issues (5.2.x) #23093

Closed

alxhub closed this in 8c10df3 Mar 30, 2018

alxhub pushed a commit that referenced this pull request Mar 30, 2018

refactor(aio): move deployment config tests and helpers around (#22483)

120673a

This commit prepares the ground for adding different types of tests. PR Close #22483

alxhub pushed a commit that referenced this pull request Mar 30, 2018

fix(aio): fix SW routing RegExp to allow redirecting /api/animate U…

a9e05ac

…RLs (#22483) PR Close #22483

gkalpak deleted the ci-aio-monitoring branch April 2, 2018 07:13

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

		@@ -1,8 +1,8 @@
		import { getRedirector, loadLegacyUrls, loadRedirects, loadSitemapUrls } from './helpers';
		import { getRedirector, loadLegacyUrls, loadRedirects, loadLocalSitemapUrls } from '../shared/helpers';

		"build-ie-polyfills": "node node_modules/webpack/bin/webpack.js -p src/ie-polyfills.js src/generated/ie-polyfills.min.js",
		"update-webdriver": "webdriver-manager update --standalone false --gecko false $CHROMEDRIVER_VERSION_ARG",

aio: add monitoring and fix a couple of issues #22483

aio: add monitoring and fix a couple of issues #22483

Conversation

gkalpak commented Feb 27, 2018 • edited Loading

mary-poppins commented Feb 27, 2018

petebacondarwin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Feb 28, 2018

mary-poppins commented Feb 28, 2018

gkalpak commented Feb 28, 2018

mary-poppins commented Feb 28, 2018

IgorMinar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Mar 5, 2018

mary-poppins commented Mar 5, 2018

alxhub commented Mar 29, 2018

gkalpak commented Mar 30, 2018 • edited Loading

angular-automatic-lock-bot bot commented Sep 13, 2019

gkalpak commented Feb 27, 2018 •

edited

Loading

petebacondarwin left a comment •

edited

Loading

petebacondarwin Feb 28, 2018 •

edited

Loading

gkalpak commented Mar 30, 2018 •

edited

Loading