From 9e4520367931268274d28928aaa0cbf8fda4f1d5 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Wed, 20 Nov 2019 17:27:02 +0200 Subject: [PATCH] test(docs-infra): more thoroughly clean up after `ScrollService` tests (#33937) By clearing `sessionStorage` and unsubscribing from `Location` events, after each test, we reduce the possibility of potential [spooky action at a distance][1]-type of failures in the future. This does not have an impact on the actual app, since `ScrollService` is currently expected to live throughout the lifetime of the app. Still, unsubscribing from `Location` events keeps the code consistent with how other `ScrollService` listeners are handled (e.g. for `window` events). [1]: https://en.wikipedia.org/wiki/Action_at_a_distance_(computer_programming) PR Close #33937 --- aio/src/app/shared/scroll.service.spec.ts | 104 ++++++++++++++++------ aio/src/app/shared/scroll.service.ts | 13 +-- 2 files changed, 83 insertions(+), 34 deletions(-) diff --git a/aio/src/app/shared/scroll.service.spec.ts b/aio/src/app/shared/scroll.service.spec.ts index 5e2e4c4de9..28733e5574 100644 --- a/aio/src/app/shared/scroll.service.spec.ts +++ b/aio/src/app/shared/scroll.service.spec.ts @@ -62,7 +62,10 @@ describe('ScrollService', () => { spyOn(window, 'scrollBy'); }); - afterEach(() => scrollServiceInstances.forEach(instance => instance.ngOnDestroy())); + afterEach(() => { + scrollServiceInstances.forEach(instance => instance.ngOnDestroy()); + window.sessionStorage.clear(); + }); it('should debounce `updateScrollPositonInHistory()`', fakeAsync(() => { const updateScrollPositionInHistorySpy = spyOn(scrollService, 'updateScrollPositionInHistory'); @@ -78,25 +81,6 @@ describe('ScrollService', () => { expect(updateScrollPositionInHistorySpy).toHaveBeenCalledTimes(1); })); - it('should stop updating scroll position once destroyed', fakeAsync(() => { - const updateScrollPositionInHistorySpy = spyOn(scrollService, 'updateScrollPositionInHistory'); - - window.dispatchEvent(new Event('scroll')); - tick(250); - expect(updateScrollPositionInHistorySpy).toHaveBeenCalledTimes(1); - - window.dispatchEvent(new Event('scroll')); - tick(250); - expect(updateScrollPositionInHistorySpy).toHaveBeenCalledTimes(2); - - updateScrollPositionInHistorySpy.calls.reset(); - scrollService.ngOnDestroy(); - - window.dispatchEvent(new Event('scroll')); - tick(250); - expect(updateScrollPositionInHistorySpy).not.toHaveBeenCalled(); - })); - it('should set `scrollRestoration` to `manual` if supported', () => { if (scrollService.supportManualScrollRestoration) { expect(window.history.scrollRestoration).toBe('manual'); @@ -106,18 +90,26 @@ describe('ScrollService', () => { }); it('should not break when cookies are disabled in the browser', () => { - // Simulate `window.sessionStorage` being inaccessible, when cookies are disabled. - spyOnProperty(window, 'sessionStorage', 'get').and.throwError('The operation is insecure'); - expect(() => { - const platformLoc = platformLocation as PlatformLocation; - const service = createScrollService(document, platformLoc, viewportScrollerStub, location); + const originalSessionStorage = Object.getOwnPropertyDescriptor(window, 'sessionStorage')!; - service.updateScrollLocationHref(); - expect(service.getStoredScrollLocationHref()).toBeNull(); + try { + // Simulate `window.sessionStorage` being inaccessible, when cookies are disabled. + Object.defineProperty(window, 'sessionStorage', { + get() { throw new Error('The operation is insecure'); }, + }); - service.removeStoredScrollInfo(); - expect(service.getStoredScrollPosition()).toBeNull(); + const platformLoc = platformLocation as PlatformLocation; + const service = createScrollService(document, platformLoc, viewportScrollerStub, location); + + service.updateScrollLocationHref(); + expect(service.getStoredScrollLocationHref()).toBeNull(); + + service.removeStoredScrollInfo(); + expect(service.getStoredScrollPosition()).toBeNull(); + } finally { + Object.defineProperty(window, 'sessionStorage', originalSessionStorage); + } }).not.toThrow(); }); @@ -433,4 +425,58 @@ describe('ScrollService', () => { expect(scrollToTopSpy).not.toHaveBeenCalled(); })); }); + + describe('once destroyed', () => { + it('should stop updating scroll position', fakeAsync(() => { + const updateScrollPositionInHistorySpy = + spyOn(scrollService, 'updateScrollPositionInHistory'); + + window.dispatchEvent(new Event('scroll')); + tick(250); + expect(updateScrollPositionInHistorySpy).toHaveBeenCalledTimes(1); + + window.dispatchEvent(new Event('scroll')); + tick(250); + expect(updateScrollPositionInHistorySpy).toHaveBeenCalledTimes(2); + + updateScrollPositionInHistorySpy.calls.reset(); + scrollService.ngOnDestroy(); + + window.dispatchEvent(new Event('scroll')); + tick(250); + expect(updateScrollPositionInHistorySpy).not.toHaveBeenCalled(); + })); + + it('should stop updating the stored location href', () => { + const updateScrollLocationHrefSpy = spyOn(scrollService, 'updateScrollLocationHref'); + + window.dispatchEvent(new Event('beforeunload')); + expect(updateScrollLocationHrefSpy).toHaveBeenCalledTimes(1); + + window.dispatchEvent(new Event('beforeunload')); + expect(updateScrollLocationHrefSpy).toHaveBeenCalledTimes(2); + + updateScrollLocationHrefSpy.calls.reset(); + scrollService.ngOnDestroy(); + + window.dispatchEvent(new Event('beforeunload')); + expect(updateScrollLocationHrefSpy).not.toHaveBeenCalled(); + }); + + it('should stop scrolling on `hashchange` events', () => { + const scrollToPositionSpy = spyOn(scrollService, 'scrollToPosition'); + + location.simulateHashChange('foo'); + expect(scrollToPositionSpy).toHaveBeenCalledTimes(1); + + location.simulateHashChange('bar'); + expect(scrollToPositionSpy).toHaveBeenCalledTimes(2); + + scrollToPositionSpy.calls.reset(); + scrollService.ngOnDestroy(); + + location.simulateHashChange('baz'); + expect(scrollToPositionSpy).not.toHaveBeenCalled(); + }); + }); }); diff --git a/aio/src/app/shared/scroll.service.ts b/aio/src/app/shared/scroll.service.ts index 18295cf31e..436040c8d3 100644 --- a/aio/src/app/shared/scroll.service.ts +++ b/aio/src/app/shared/scroll.service.ts @@ -78,13 +78,14 @@ export class ScrollService implements OnDestroy { .pipe(takeUntil(this.onDestroy)) .subscribe(() => this.updateScrollLocationHref()); - // Change scroll restoration strategy to `manual` if it's supported + // Change scroll restoration strategy to `manual` if it's supported. if (this.supportManualScrollRestoration) { history.scrollRestoration = 'manual'; - // we have to detect forward and back navigation thanks to popState event - this.location.subscribe((event: ScrollPositionPopStateEvent) => { - // the type is `hashchange` when the fragment identifier of the URL has changed. It allows us to go to position - // just before a click on an anchor + + // We have to detect forward and back navigation thanks to popState event. + const locationSubscription = this.location.subscribe((event: ScrollPositionPopStateEvent) => { + // The type is `hashchange` when the fragment identifier of the URL has changed. It allows + // us to go to position just before a click on an anchor. if (event.type === 'hashchange') { this.scrollToPosition(); } else { @@ -96,6 +97,8 @@ export class ScrollService implements OnDestroy { this.poppedStateScrollPosition = event.state ? event.state.scrollPosition : null; } }); + + this.onDestroy.subscribe(() => locationSubscription.unsubscribe()); } // If this was not a reload, discard the stored scroll info.