From 431a8194633a9e5eff28353bdc884d65bc573194 Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Thu, 9 Apr 2020 10:08:21 -0400 Subject: [PATCH 1/4] Disable auto reload when page is not visible in ng_tensorboard. --- .../webapp/reloader/reloader_component.ts | 32 +++- .../reloader/reloader_component_test.ts | 171 +++++++++++++++++- 2 files changed, 200 insertions(+), 3 deletions(-) diff --git a/tensorboard/webapp/reloader/reloader_component.ts b/tensorboard/webapp/reloader/reloader_component.ts index 8a854c7e64..95373fc338 100644 --- a/tensorboard/webapp/reloader/reloader_component.ts +++ b/tensorboard/webapp/reloader/reloader_component.ts @@ -22,21 +22,34 @@ import {reload} from '../core/actions'; /** @typehack */ import * as _typeHackRxjs from 'rxjs'; +const documentUtil = { + /** + * Wraps Page Visibility API call to determine if document is visible. + * Can be overriden for testing purposes. + */ + getVisibilityState(): string { + return document.visibilityState; + }, +}; + @Component({ selector: 'reloader', template: '', changeDetection: ChangeDetectionStrategy.OnPush, }) export class ReloaderComponent { + private readonly onVisibilityChange = this.onVisibilityChangeImpl.bind(this); private readonly reloadEnabled$ = this.store.pipe(select(getReloadEnabled)); private readonly reloadPeriodInMs$ = this.store.pipe( select(getReloadPeriodInMs) ); private reloadTimerId: ReturnType | null = null; + private missedAutoReload: boolean = false; constructor(private store: Store) {} ngOnInit() { + window.addEventListener('visibilitychange', this.onVisibilityChange); combineLatest( this.reloadEnabled$.pipe(distinctUntilChanged()), this.reloadPeriodInMs$.pipe(distinctUntilChanged()) @@ -48,9 +61,23 @@ export class ReloaderComponent { }); } + private onVisibilityChangeImpl() { + if ( + documentUtil.getVisibilityState() === 'visible' && + this.missedAutoReload + ) { + this.missedAutoReload = false; + this.store.dispatch(reload()); + } + } + private load(reloadPeriodInMs: number) { this.reloadTimerId = setTimeout(() => { - this.store.dispatch(reload()); + if (documentUtil.getVisibilityState() === 'visible') { + this.store.dispatch(reload()); + } else { + this.missedAutoReload = true; + } this.load(reloadPeriodInMs); }, reloadPeriodInMs); } @@ -64,5 +91,8 @@ export class ReloaderComponent { ngOnDestroy() { this.cancelLoad(); + window.removeEventListener('visibilitychange', this.onVisibilityChange); } } + +export const TEST_ONLY = {documentUtil}; diff --git a/tensorboard/webapp/reloader/reloader_component_test.ts b/tensorboard/webapp/reloader/reloader_component_test.ts index 1ce126486d..24e5f93dcb 100644 --- a/tensorboard/webapp/reloader/reloader_component_test.ts +++ b/tensorboard/webapp/reloader/reloader_component_test.ts @@ -16,7 +16,7 @@ import {TestBed, fakeAsync, tick} from '@angular/core/testing'; import {Store} from '@ngrx/store'; import {provideMockStore, MockStore} from '@ngrx/store/testing'; -import {ReloaderComponent} from './reloader_component'; +import {ReloaderComponent, TEST_ONLY} from './reloader_component'; import {reload} from '../core/actions'; import {State} from '../core/store'; @@ -27,6 +27,9 @@ import {createState, createCoreState} from '../core/testing'; describe('reloader_component', () => { let store: MockStore; let dispatchSpy: jasmine.Spy; + // Specifies whether stubbed documentUtil.getVisibilityState will return + // 'visible' (true) or 'hidden' (false) + let isDocumentVisible: boolean = true; beforeEach(async () => { await TestBed.configureTestingModule({ @@ -45,8 +48,17 @@ describe('reloader_component', () => { }).compileComponents(); store = TestBed.get(Store); dispatchSpy = spyOn(store, 'dispatch'); + isDocumentVisible = true; + spyOn(TEST_ONLY.documentUtil, 'getVisibilityState').and.callFake(() => + isDocumentVisible ? 'visible' : 'hidden' + ); }); + function simulateVisibilityChange(visibility: boolean) { + isDocumentVisible = visibility; + window.dispatchEvent(new Event('visibilitychange')); + } + it('dispatches reload action every reload period', fakeAsync(() => { store.setState( createState( @@ -69,7 +81,8 @@ describe('reloader_component', () => { expect(dispatchSpy).toHaveBeenCalledTimes(2); expect(dispatchSpy).toHaveBeenCalledWith(reload()); - // // Manually invoke destruction of the component so we can cleanup the timer. + // Manually invoke destruction of the component so we can cleanup the + // timer. fixture.destroy(); })); @@ -161,4 +174,158 @@ describe('reloader_component', () => { fixture.destroy(); })); + + it('does not reload if document is not visible', fakeAsync(() => { + store.setState( + createState( + createCoreState({ + reloadPeriodInMs: 5, + reloadEnabled: true, + }) + ) + ); + const fixture = TestBed.createComponent(ReloaderComponent); + fixture.detectChanges(); + + tick(5); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + + simulateVisibilityChange(false); + tick(5); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + + fixture.destroy(); + })); + + it('reloads when document becomes visible if missed reload', fakeAsync(() => { + store.setState( + createState( + createCoreState({ + reloadPeriodInMs: 5, + reloadEnabled: true, + }) + ) + ); + const fixture = TestBed.createComponent(ReloaderComponent); + fixture.detectChanges(); + + // Miss a reload because not visible. + simulateVisibilityChange(false); + tick(5); + expect(dispatchSpy).toHaveBeenCalledTimes(0); + + // Dispatch a reload when next visible. + simulateVisibilityChange(true); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + + fixture.destroy(); + })); + + it('reloads when document becomes visible if missed reload, regardless of how long not visible', fakeAsync(() => { + store.setState( + createState( + createCoreState({ + reloadPeriodInMs: 5, + reloadEnabled: true, + }) + ) + ); + const fixture = TestBed.createComponent(ReloaderComponent); + fixture.detectChanges(); + + tick(5); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + + // Document is not visible during time period that includes missed auto + // reload but is less than reloadPeriodInMs. + tick(2); + simulateVisibilityChange(false); + tick(3); + // No reload is dispatched. + expect(dispatchSpy).toHaveBeenCalledTimes(1); + + // Dispatch a reload when next visible. + simulateVisibilityChange(true); + expect(dispatchSpy).toHaveBeenCalledTimes(2); + + fixture.destroy(); + })); + + it('does not reload when document becomes visible if there was not a missed reload', fakeAsync(() => { + store.setState( + createState( + createCoreState({ + reloadPeriodInMs: 5, + reloadEnabled: true, + }) + ) + ); + const fixture = TestBed.createComponent(ReloaderComponent); + fixture.detectChanges(); + + tick(5); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + + // Document is not visible during time period that does not include + // missed auto reload. + simulateVisibilityChange(false); + tick(3); + simulateVisibilityChange(true); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + + fixture.destroy(); + })); + + it('does not reload when document becomes visible if missed reload was already handled', fakeAsync(() => { + store.setState( + createState( + createCoreState({ + reloadPeriodInMs: 5, + reloadEnabled: true, + }) + ) + ); + const fixture = TestBed.createComponent(ReloaderComponent); + fixture.detectChanges(); + + // Miss a reload because not visible. + simulateVisibilityChange(false); + tick(6); + expect(dispatchSpy).toHaveBeenCalledTimes(0); + + // Dispatch a reload when next visible. + simulateVisibilityChange(true); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + + // Document is not visible during time period that does not include + // another missed reload. + simulateVisibilityChange(false); + tick(2); + simulateVisibilityChange(true); + // No additional reload dispatched. + expect(dispatchSpy).toHaveBeenCalledTimes(1); + + fixture.destroy(); + })); + + it('does not reload when document becomes visible if auto reload is off', fakeAsync(() => { + store.setState( + createState( + createCoreState({ + reloadPeriodInMs: 5, + reloadEnabled: false, + }) + ) + ); + const fixture = TestBed.createComponent(ReloaderComponent); + fixture.detectChanges(); + + simulateVisibilityChange(false); + tick(5); + + simulateVisibilityChange(true); + expect(dispatchSpy).toHaveBeenCalledTimes(0); + + fixture.destroy(); + })); }); From cc8d84af127ed25b5ae6f956c2c0be5f9da315dc Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Fri, 10 Apr 2020 13:12:59 -0400 Subject: [PATCH 2/4] Inject document into ReloaderComponent. --- .../webapp/reloader/reloader_component.ts | 34 +++++++------------ .../reloader/reloader_component_test.ts | 30 ++++++++++------ 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/tensorboard/webapp/reloader/reloader_component.ts b/tensorboard/webapp/reloader/reloader_component.ts index 95373fc338..8e217aaeb5 100644 --- a/tensorboard/webapp/reloader/reloader_component.ts +++ b/tensorboard/webapp/reloader/reloader_component.ts @@ -12,7 +12,8 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import {Component, ChangeDetectionStrategy} from '@angular/core'; +import {DOCUMENT} from '@angular/common'; +import {Component, ChangeDetectionStrategy, Inject} from '@angular/core'; import {Store, select} from '@ngrx/store'; import {combineLatest} from 'rxjs'; import {distinctUntilChanged} from 'rxjs/operators'; @@ -22,16 +23,6 @@ import {reload} from '../core/actions'; /** @typehack */ import * as _typeHackRxjs from 'rxjs'; -const documentUtil = { - /** - * Wraps Page Visibility API call to determine if document is visible. - * Can be overriden for testing purposes. - */ - getVisibilityState(): string { - return document.visibilityState; - }, -}; - @Component({ selector: 'reloader', template: '', @@ -46,10 +37,13 @@ export class ReloaderComponent { private reloadTimerId: ReturnType | null = null; private missedAutoReload: boolean = false; - constructor(private store: Store) {} + constructor( + private store: Store, + @Inject(DOCUMENT) private document: Document + ) {} ngOnInit() { - window.addEventListener('visibilitychange', this.onVisibilityChange); + this.document.addEventListener('visibilitychange', this.onVisibilityChange); combineLatest( this.reloadEnabled$.pipe(distinctUntilChanged()), this.reloadPeriodInMs$.pipe(distinctUntilChanged()) @@ -62,10 +56,7 @@ export class ReloaderComponent { } private onVisibilityChangeImpl() { - if ( - documentUtil.getVisibilityState() === 'visible' && - this.missedAutoReload - ) { + if (this.document.visibilityState === 'visible' && this.missedAutoReload) { this.missedAutoReload = false; this.store.dispatch(reload()); } @@ -73,7 +64,7 @@ export class ReloaderComponent { private load(reloadPeriodInMs: number) { this.reloadTimerId = setTimeout(() => { - if (documentUtil.getVisibilityState() === 'visible') { + if (this.document.visibilityState === 'visible') { this.store.dispatch(reload()); } else { this.missedAutoReload = true; @@ -91,8 +82,9 @@ export class ReloaderComponent { ngOnDestroy() { this.cancelLoad(); - window.removeEventListener('visibilitychange', this.onVisibilityChange); + this.document.removeEventListener( + 'visibilitychange', + this.onVisibilityChange + ); } } - -export const TEST_ONLY = {documentUtil}; diff --git a/tensorboard/webapp/reloader/reloader_component_test.ts b/tensorboard/webapp/reloader/reloader_component_test.ts index 24e5f93dcb..f50161893b 100644 --- a/tensorboard/webapp/reloader/reloader_component_test.ts +++ b/tensorboard/webapp/reloader/reloader_component_test.ts @@ -12,11 +12,12 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ +import {DOCUMENT} from '@angular/common'; import {TestBed, fakeAsync, tick} from '@angular/core/testing'; import {Store} from '@ngrx/store'; import {provideMockStore, MockStore} from '@ngrx/store/testing'; -import {ReloaderComponent, TEST_ONLY} from './reloader_component'; +import {ReloaderComponent} from './reloader_component'; import {reload} from '../core/actions'; import {State} from '../core/store'; @@ -27,13 +28,24 @@ import {createState, createCoreState} from '../core/testing'; describe('reloader_component', () => { let store: MockStore; let dispatchSpy: jasmine.Spy; - // Specifies whether stubbed documentUtil.getVisibilityState will return - // 'visible' (true) or 'hidden' (false) - let isDocumentVisible: boolean = true; + + const fakeDocument = { + visibilityState: 'visible', + addEventListener: document.addEventListener.bind(document), + removeEventListener: document.removeEventListener.bind(document), + // DOMTestComponentRenderer injects DOCUMENT and requires the following + // properties to function. + querySelectorAll: document.querySelectorAll.bind(document), + body: document.body, + }; beforeEach(async () => { await TestBed.configureTestingModule({ providers: [ + { + provide: DOCUMENT, + useFactory: () => fakeDocument, + }, provideMockStore({ initialState: createState( createCoreState({ @@ -48,15 +60,11 @@ describe('reloader_component', () => { }).compileComponents(); store = TestBed.get(Store); dispatchSpy = spyOn(store, 'dispatch'); - isDocumentVisible = true; - spyOn(TEST_ONLY.documentUtil, 'getVisibilityState').and.callFake(() => - isDocumentVisible ? 'visible' : 'hidden' - ); }); - function simulateVisibilityChange(visibility: boolean) { - isDocumentVisible = visibility; - window.dispatchEvent(new Event('visibilitychange')); + function simulateVisibilityChange(visible: boolean) { + fakeDocument.visibilityState = visible ? 'visible' : 'hidden'; + document.dispatchEvent(new Event('visibilitychange')); } it('dispatches reload action every reload period', fakeAsync(() => { From 4e131236c8647328ec67f9026e0e71d28f414563 Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Mon, 13 Apr 2020 10:50:37 -0400 Subject: [PATCH 3/4] Respond to PR comments * document is readonly property in ReloaderComponent * fakeDocument is created for each test run --- .../webapp/reloader/reloader_component.ts | 2 +- .../reloader/reloader_component_test.ts | 34 +++++++++++-------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/tensorboard/webapp/reloader/reloader_component.ts b/tensorboard/webapp/reloader/reloader_component.ts index 8e217aaeb5..45fb01a65d 100644 --- a/tensorboard/webapp/reloader/reloader_component.ts +++ b/tensorboard/webapp/reloader/reloader_component.ts @@ -39,7 +39,7 @@ export class ReloaderComponent { constructor( private store: Store, - @Inject(DOCUMENT) private document: Document + @Inject(DOCUMENT) private readonly document: Document ) {} ngOnInit() { diff --git a/tensorboard/webapp/reloader/reloader_component_test.ts b/tensorboard/webapp/reloader/reloader_component_test.ts index f50161893b..3b394fa16b 100644 --- a/tensorboard/webapp/reloader/reloader_component_test.ts +++ b/tensorboard/webapp/reloader/reloader_component_test.ts @@ -28,23 +28,31 @@ import {createState, createCoreState} from '../core/testing'; describe('reloader_component', () => { let store: MockStore; let dispatchSpy: jasmine.Spy; + let fakeDocument: any; + + function createFakeDocument() { + return { + visibilityState: 'visible', + addEventListener: document.addEventListener.bind(document), + removeEventListener: document.removeEventListener.bind(document), + // DOMTestComponentRenderer injects DOCUMENT and requires the following + // properties to function. + querySelectorAll: document.querySelectorAll.bind(document), + body: document.body, + }; + } - const fakeDocument = { - visibilityState: 'visible', - addEventListener: document.addEventListener.bind(document), - removeEventListener: document.removeEventListener.bind(document), - // DOMTestComponentRenderer injects DOCUMENT and requires the following - // properties to function. - querySelectorAll: document.querySelectorAll.bind(document), - body: document.body, - }; + function simulateVisibilityChange(visible: boolean) { + fakeDocument.visibilityState = visible ? 'visible' : 'hidden'; + document.dispatchEvent(new Event('visibilitychange')); + } beforeEach(async () => { await TestBed.configureTestingModule({ providers: [ { provide: DOCUMENT, - useFactory: () => fakeDocument, + useFactory: createFakeDocument, }, provideMockStore({ initialState: createState( @@ -59,14 +67,10 @@ describe('reloader_component', () => { declarations: [ReloaderComponent], }).compileComponents(); store = TestBed.get(Store); + fakeDocument = TestBed.get(DOCUMENT); dispatchSpy = spyOn(store, 'dispatch'); }); - function simulateVisibilityChange(visible: boolean) { - fakeDocument.visibilityState = visible ? 'visible' : 'hidden'; - document.dispatchEvent(new Event('visibilitychange')); - } - it('dispatches reload action every reload period', fakeAsync(() => { store.setState( createState( From 0dc5fd24bcd4df80eacab449c02c98b5030c17d1 Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Wed, 15 Apr 2020 11:28:18 -0400 Subject: [PATCH 4/4] Improve typing of fakeDocument. --- tensorboard/webapp/reloader/reloader_component_test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tensorboard/webapp/reloader/reloader_component_test.ts b/tensorboard/webapp/reloader/reloader_component_test.ts index 3b394fa16b..b82894f6eb 100644 --- a/tensorboard/webapp/reloader/reloader_component_test.ts +++ b/tensorboard/webapp/reloader/reloader_component_test.ts @@ -28,7 +28,7 @@ import {createState, createCoreState} from '../core/testing'; describe('reloader_component', () => { let store: MockStore; let dispatchSpy: jasmine.Spy; - let fakeDocument: any; + let fakeDocument: Document; function createFakeDocument() { return { @@ -43,7 +43,9 @@ describe('reloader_component', () => { } function simulateVisibilityChange(visible: boolean) { - fakeDocument.visibilityState = visible ? 'visible' : 'hidden'; + Object.defineProperty(fakeDocument, 'visibilityState', { + get: () => (visible ? 'visible' : 'hidden'), + }); document.dispatchEvent(new Event('visibilitychange')); }