From d0a7292b33c3494b088f2cdfbb9dad20dd824f6f Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Mon, 18 Jul 2022 13:34:20 -0700 Subject: [PATCH] rewrite tests to avoid using returnValues which for some reason fails --- tensorboard/webapp/app_routing/location.ts | 2 +- .../routes/dashboard_deeplink_provider.ts | 16 +- .../webapp/routes/feature_flag_serializer.ts | 53 ++-- .../routes/feature_flag_serializer_test.ts | 4 +- .../tb_feature_flag_data_source.ts | 17 +- .../tb_feature_flag_data_source_test.ts | 250 ++++++++++++------ 6 files changed, 215 insertions(+), 127 deletions(-) diff --git a/tensorboard/webapp/app_routing/location.ts b/tensorboard/webapp/app_routing/location.ts index 520c4d8cf70..bf1cd56fa02 100644 --- a/tensorboard/webapp/app_routing/location.ts +++ b/tensorboard/webapp/app_routing/location.ts @@ -50,7 +50,7 @@ const utils = { }, getSearch() { return window.location.search; - } + }, }; @Injectable() diff --git a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts index eb5695de35e..8333de18975 100644 --- a/tensorboard/webapp/routes/dashboard_deeplink_provider.ts +++ b/tensorboard/webapp/routes/dashboard_deeplink_provider.ts @@ -14,7 +14,7 @@ limitations under the License. ==============================================================================*/ import {Injectable} from '@angular/core'; import {Store} from '@ngrx/store'; -import {combineLatest, of, Observable} from 'rxjs'; +import {combineLatest, Observable, of} from 'rxjs'; import {map} from 'rxjs/operators'; import {DeepLinkProvider} from '../app_routing/deep_link_provider'; import {SerializableQueryParams} from '../app_routing/types'; @@ -97,12 +97,14 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { return [{key: TAG_FILTER_KEY, value: filterText}]; }) ), - of(getOverriddenFeatureFlagStates( - FeatureFlagMetadataMap as Record< - string, - FeatureFlagMetadata - > - )), + of( + getOverriddenFeatureFlagStates( + FeatureFlagMetadataMap as Record< + string, + FeatureFlagMetadata + > + ) + ), store.select(selectors.getMetricsSettingOverrides).pipe( map((settingOverrides) => { if (Number.isFinite(settingOverrides.scalarSmoothing)) { diff --git a/tensorboard/webapp/routes/feature_flag_serializer.ts b/tensorboard/webapp/routes/feature_flag_serializer.ts index 249b33deb66..0249d7dcaaa 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer.ts @@ -16,31 +16,48 @@ import {Location} from '../app_routing/location'; import {SerializableQueryParams} from '../app_routing/types'; import {FeatureFlagMetadata} from '../feature_flag/store/feature_flag_metadata'; +/** + * Finds all FeatureFlags from the provided FeatureFlagMetadata that are present in the + * query params and returns their overridden values. + * Note: If a flag has multiple values in the query params it will be returned multiple + * times. + * + * i.e. The query params '?experimentalPlugin=0&experimentalPlugin=1&experimentalPlugin=2' + * will result in a return value of + * [ + * { key: 'experimentalPlugin', value: '0' }, + * { key: 'experimentalPlugin', value: '1' }, + * { key: 'experimentalPlugin', value: '2' }, + * ] + */ export function getOverriddenFeatureFlagStates( - featureFlagQueryParameters: Record> + featureFlagMetadataMap: Record> ): SerializableQueryParams { - const currentQueryParams = Object.fromEntries( - serializableQueryParamsToEntries(new Location().getSearch() || []) + // Converting the array to a map allows for a more efficient filter function below. + const currentQueryParams = (new Location().getSearch() || []).reduce( + (map, {key, value}) => { + if (!map[key]) { + map[key] = []; + } + map[key].push(value); + + return map; + }, + {} as Record ); - const currentlyOverriddenQueryParams = Object.values( - featureFlagQueryParameters - ) + const currentlyOverriddenQueryParams = Object.values(featureFlagMetadataMap) .map(({queryParamOverride}: FeatureFlagMetadata) => queryParamOverride) .filter( (queryParamOverride) => queryParamOverride && queryParamOverride in currentQueryParams ) as string[]; - return currentlyOverriddenQueryParams.map((queryParamOverride) => { - return { - key: queryParamOverride, - value: currentQueryParams[queryParamOverride], - }; - }); - - function serializableQueryParamsToEntries( - params: SerializableQueryParams - ): [string, string][] { - return params.map(({key, value}) => [key, value]); - } + return currentlyOverriddenQueryParams + .map((queryParamOverride) => { + return currentQueryParams[queryParamOverride].map((value) => ({ + key: queryParamOverride, + value, + })); + }) + .flat(); } diff --git a/tensorboard/webapp/routes/feature_flag_serializer_test.ts b/tensorboard/webapp/routes/feature_flag_serializer_test.ts index 985f19d533d..c49ddb19cc2 100644 --- a/tensorboard/webapp/routes/feature_flag_serializer_test.ts +++ b/tensorboard/webapp/routes/feature_flag_serializer_test.ts @@ -59,9 +59,7 @@ describe('feature flag serializer', () => { }); it('persists flag states overridden by query params', async () => { - spyOn(TEST_ONLY.utils, 'getSearch').and.returnValue( - '?darkMode=true' - ); + spyOn(TEST_ONLY.utils, 'getSearch').and.returnValue('?darkMode=true'); console.log(location.getSearch()); const queryParams = getOverriddenFeatureFlagStates( FeatureFlagMetadataMap as Record< diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts index 92de9375ab1..fb3e73b5d6e 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts @@ -59,20 +59,17 @@ export class QueryParamsFeatureFlagDataSource if (!queryParamOverride || !params.has(queryParamOverride)) { return null; } - const paramValues: T[] = this.queryParams - .getParams() - .getAll(queryParamOverride) - .map((value) => { - if (value === '' && flagMetadata.defaultValue !== undefined) { - return flagMetadata.defaultValue; - } - return flagMetadata.parseValue(value); - }); + const paramValues: T[] = params.getAll(queryParamOverride).map((value) => { + if (value === '' && flagMetadata.defaultValue !== undefined) { + return flagMetadata.defaultValue; + } + return flagMetadata.parseValue(value); + }); if (!paramValues.length) { return null; } - return flagMetadata.isArray ? paramValues : paramValues[0]; + return flagMetadata.isArray ? paramValues.flat() : paramValues[0]; } protected getPartialFeaturesFromMediaQuery(): { diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts index f054f64f2ef..cde48c6c375 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts @@ -84,123 +84,197 @@ describe('tb_feature_flag_data_source', () => { }); }); - it('returns enableColorGroup from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('enableColorGroup=false'), - new URLSearchParams('enableColorGroup='), - new URLSearchParams('enableColorGroup=true'), - new URLSearchParams('enableColorGroup=foo') - ); - expect(dataSource.getFeatures()).toEqual({enabledColorGroup: false}); - expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); - expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); - expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); + describe('returns enableColorGroup from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroup=false') + ); + expect(dataSource.getFeatures()).toEqual({enabledColorGroup: false}); + }); + + it('when set to empty string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroup=') + ); + expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); + }); + + it('when set to true', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroup=true') + ); + expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); + }); + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroup=foo') + ); + expect(dataSource.getFeatures()).toEqual({enabledColorGroup: true}); + }); }); - it('returns enableColorGroupByRegex from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('enableColorGroupByRegex=false'), - new URLSearchParams('enableColorGroupByRegex='), - new URLSearchParams('enableColorGroupByRegex=true'), - new URLSearchParams('enableColorGroupByRegex=foo') - ); - expect(dataSource.getFeatures()).toEqual({ - enabledColorGroupByRegex: false, + describe('returns enabledColorGroupByRegex from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroupByRegex=false') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledColorGroupByRegex: false, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledColorGroupByRegex: true, + + it('when set to empty string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroupByRegex=') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledColorGroupByRegex: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledColorGroupByRegex: true, + + it('when set to true', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroupByRegex=true') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledColorGroupByRegex: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledColorGroupByRegex: true, + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableColorGroupByRegex=foo') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledColorGroupByRegex: true, + }); }); }); - it('returns enableLinkedTime from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('enableLinkedTime=false'), - new URLSearchParams('enableLinkedTime='), - new URLSearchParams('enableLinkedTime=true'), - new URLSearchParams('enableLinkedTime=foo') - ); - expect(dataSource.getFeatures()).toEqual({ - enabledLinkedTime: false, + describe('returns enabledLinkedTime from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableLinkedTime=false') + ); + expect(dataSource.getFeatures()).toEqual({enabledLinkedTime: false}); }); - expect(dataSource.getFeatures()).toEqual({ - enabledLinkedTime: true, + + it('when set to empty string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableLinkedTime=') + ); + expect(dataSource.getFeatures()).toEqual({enabledLinkedTime: true}); }); - expect(dataSource.getFeatures()).toEqual({ - enabledLinkedTime: true, + + it('when set to true', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableLinkedTime=true') + ); + expect(dataSource.getFeatures()).toEqual({enabledLinkedTime: true}); }); - expect(dataSource.getFeatures()).toEqual({ - enabledLinkedTime: true, + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableLinkedTime=foo') + ); + expect(dataSource.getFeatures()).toEqual({enabledLinkedTime: true}); }); }); - it('returns enableCardWidthSetting from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('enableCardWidthSetting=false'), - new URLSearchParams('enableCardWidthSetting='), - new URLSearchParams('enableCardWidthSetting=true'), - new URLSearchParams('enableCardWidthSetting=foo') - ); - - expect(dataSource.getFeatures()).toEqual({ - enabledCardWidthSetting: false, + describe('returns enabledCardWidthSetting from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableCardWidthSetting=false') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledCardWidthSetting: false, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledCardWidthSetting: true, + + it('when set to empty string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableCardWidthSetting=') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledCardWidthSetting: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledCardWidthSetting: true, + + it('when set to true', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableCardWidthSetting=true') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledCardWidthSetting: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledCardWidthSetting: true, + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableCardWidthSetting=foo') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledCardWidthSetting: true, + }); }); }); - it('returns forceSVG from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('forceSVG=false'), - new URLSearchParams('forceSVG='), - new URLSearchParams('forceSVG=true'), - new URLSearchParams('forceSVG=foo') - ); - - expect(dataSource.getFeatures()).toEqual({ - forceSvg: false, + describe('returns forceSvg from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue(new URLSearchParams('forceSVG=false')); + expect(dataSource.getFeatures()).toEqual({forceSvg: false}); }); - expect(dataSource.getFeatures()).toEqual({ - forceSvg: true, + + it('when set to empty string', () => { + getParamsSpy.and.returnValue(new URLSearchParams('forceSVG=')); + expect(dataSource.getFeatures()).toEqual({forceSvg: true}); }); - expect(dataSource.getFeatures()).toEqual({ - forceSvg: true, + + it('when set to true', () => { + getParamsSpy.and.returnValue(new URLSearchParams('forceSVG=true')); + expect(dataSource.getFeatures()).toEqual({forceSvg: true}); }); - expect(dataSource.getFeatures()).toEqual({ - forceSvg: true, + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue(new URLSearchParams('forceSVG=foo')); + expect(dataSource.getFeatures()).toEqual({forceSvg: true}); }); }); - it('returns enableDataTable from the query params', () => { - getParamsSpy.and.returnValues( - new URLSearchParams('enableDataTable=false'), - new URLSearchParams('enableDataTable='), - new URLSearchParams('enableDataTable=true'), - new URLSearchParams('enableDataTable=foo') - ); - expect(dataSource.getFeatures()).toEqual({ - enabledScalarDataTable: false, + describe('returns enabledDataTable from the query params', () => { + it('when set to false', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableDataTable=false') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledScalarDataTable: false, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledScalarDataTable: true, + + it('when set to empty string', () => { + getParamsSpy.and.returnValue(new URLSearchParams('enableDataTable=')); + expect(dataSource.getFeatures()).toEqual({ + enabledScalarDataTable: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledScalarDataTable: true, + + it('when set to true', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableDataTable=true') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledScalarDataTable: true, + }); }); - expect(dataSource.getFeatures()).toEqual({ - enabledScalarDataTable: true, + + it('when set to an arbitrary string', () => { + getParamsSpy.and.returnValue( + new URLSearchParams('enableDataTable=foo') + ); + expect(dataSource.getFeatures()).toEqual({ + enabledScalarDataTable: true, + }); }); });