From 1e47e9369b0fe0728ba69b67bc930e960c223aea Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Mon, 14 Aug 2023 22:37:00 +0000 Subject: [PATCH] make tests more rigorous as per pr comments --- tensorboard/defs/defs.bzl | 47 +++---- tensorboard/webapp/widgets/data_table/BUILD | 1 + .../widgets/data_table/filter_dialog_test.ts | 126 +++++++++++++++--- 3 files changed, 136 insertions(+), 38 deletions(-) diff --git a/tensorboard/defs/defs.bzl b/tensorboard/defs/defs.bzl index 0be01beb938..e5aebf4ede3 100644 --- a/tensorboard/defs/defs.bzl +++ b/tensorboard/defs/defs.bzl @@ -22,7 +22,6 @@ load("@npm//@bazel/concatjs:index.bzl", "karma_web_test_suite", "ts_library") load("@npm//@bazel/esbuild:index.bzl", "esbuild") load("@npm//@bazel/typescript:index.bzl", "ts_config") - def tensorboard_webcomponent_library(**kwargs): """Rules referencing this will be deleted from the codebase soon.""" pass @@ -72,8 +71,8 @@ def tf_js_binary( # the global level and tends to overwrite `window` functions. "iife" is # just a thin wrapper around "esm" (it adds 11 bytes) and doesn't # suffer from the same overwriting problem. - format="iife", - minify= False if dev_mode_only else True, + format = "iife", + minify = False if dev_mode_only else True, args = { # Must specify that 'mjs' extensions are preferred, since that is # the extension that is used for es2015/esm code generated by @@ -91,7 +90,6 @@ def tf_js_binary( **kwargs ) - def tf_ng_prod_js_binary( name, compile, @@ -114,7 +112,7 @@ def tf_ng_prod_js_binary( https://esbuild.github.io/api/ for more details. """ - app_bundle_name = '%s_app_bundle' % name + app_bundle_name = "%s_app_bundle" % name app_bundle( name = app_bundle_name, **kwargs @@ -124,8 +122,8 @@ def tf_ng_prod_js_binary( # through a terser pass to be the output of this rule. copy_file( name = name, - src = '%s.min.js' % app_bundle_name, - out = '%s.js' % name, + src = "%s.min.js" % app_bundle_name, + out = "%s.js" % name, ) def tf_ts_config(**kwargs): @@ -148,22 +146,24 @@ def tf_ts_library(srcs = [], strict_checks = True, **kwargs): kwargs.setdefault("deps", []).extend(["@npm//tslib", "//tensorboard/defs:strict_types"]) new_srcs = [] + # Find test.ts and testbed.ts files and rename to test.spec.ts to be # compatible with spec_bundle() tooling. for s in srcs: - if s.endswith("_test.ts") or s.endswith("-test.ts") or s.endswith("_testbed.ts"): - # Make a copy of the file with name .spec.ts and switch to that as - # the src file. - new_src = s[0:s.rindex('.ts')] + ".spec.ts" - copy_file( - name = new_src + '_spec_copy', - src = s, - out = new_src, - allow_symlink = True) - new_srcs.append(new_src) - else: - # Not a test file. Nothing to do here. - new_srcs.append(s) + if s.endswith("_test.ts") or s.endswith("-test.ts") or s.endswith("_testbed.ts"): + # Make a copy of the file with name .spec.ts and switch to that as + # the src file. + new_src = s[0:s.rindex(".ts")] + ".spec.ts" + copy_file( + name = new_src + "_spec_copy", + src = s, + out = new_src, + allow_symlink = True, + ) + new_srcs.append(new_src) + else: + # Not a test file. Nothing to do here. + new_srcs.append(s) ts_library( srcs = new_srcs, @@ -176,7 +176,8 @@ def tf_ts_library(srcs = [], strict_checks = True, **kwargs): prodmode_target = "es2020", devmode_target = "es2020", devmode_module = "esnext", - **kwargs) + **kwargs + ) def tf_ng_web_test_suite(name, deps = [], **kwargs): """TensorBoard wrapper for the rule for a Karma web test suite. @@ -231,7 +232,7 @@ def tf_ng_web_test_suite(name, deps = [], **kwargs): # karma_web_test_suite() will rebundle it along with the test framework # in a CommonJS bundle. deps = [ - "%s_bundle" % name, + "%s_bundle" % name, ], ) @@ -256,7 +257,7 @@ def tf_sass_binary(deps = [], include_paths = [], strict_deps = True, **kwargs): internally. """ if (strict_deps): - fail("all tf_sass_binary calls need to have the strict_deps = False override for internal calls"); + fail("all tf_sass_binary calls need to have the strict_deps = False override for internal calls") sass_binary( deps = deps, include_paths = include_paths + [ diff --git a/tensorboard/webapp/widgets/data_table/BUILD b/tensorboard/webapp/widgets/data_table/BUILD index 7209095d352..eed2d32dfbb 100644 --- a/tensorboard/webapp/widgets/data_table/BUILD +++ b/tensorboard/webapp/widgets/data_table/BUILD @@ -186,6 +186,7 @@ tf_ts_library( "//tensorboard/webapp/testing:mat_icon", "//tensorboard/webapp/widgets/custom_modal", "//tensorboard/webapp/widgets/range_input", + "//tensorboard/webapp/widgets/range_input:types", "@npm//@angular/core", "@npm//@angular/forms", "@npm//@angular/platform-browser", diff --git a/tensorboard/webapp/widgets/data_table/filter_dialog_test.ts b/tensorboard/webapp/widgets/data_table/filter_dialog_test.ts index aade7c07c3c..1dfe1ce0269 100644 --- a/tensorboard/webapp/widgets/data_table/filter_dialog_test.ts +++ b/tensorboard/webapp/widgets/data_table/filter_dialog_test.ts @@ -17,12 +17,18 @@ import {HarnessLoader} from '@angular/cdk/testing'; import {TestbedHarnessEnvironment} from '@angular/cdk/testing/testbed'; import {DataTableModule} from './data_table_module'; import {FilterDialog} from './filter_dialog_component'; -import {DiscreteFilter, DomainType, IntervalFilter} from './types'; +import { + DiscreteFilter, + DomainType, + IntervalFilter, + DiscreteFilterValue, +} from './types'; import {By} from '@angular/platform-browser'; import {RangeInputComponent} from '../range_input/range_input_component'; import {RangeInputModule} from '../range_input/range_input_module'; import {MatCheckboxModule} from '@angular/material/checkbox'; import {MatCheckboxHarness} from '@angular/material/checkbox/testing'; +import {RangeInputSource} from '../range_input/types'; describe('filter dialog', () => { let rootLoader: HarnessLoader; @@ -42,6 +48,29 @@ describe('filter dialog', () => { return fixture; } + it('renders interval filters', () => { + const fixture = createComponent({ + filter: { + type: DomainType.INTERVAL, + includeUndefined: false, + minValue: 6, + maxValue: 20, + filterLowerValue: 7, + filterUpperValue: 18, + }, + }); + + const rangeInput = fixture.debugElement.query( + By.directive(RangeInputComponent) + ); + expect(rangeInput).toBeTruthy(); + const [lower, upper, ...rest] = rangeInput.queryAll(By.css('input')); + // There should only be two input fields. + expect(rest.length).toEqual(0); + expect(lower.nativeElement.value).toEqual('7'); + expect(upper.nativeElement.value).toEqual('18'); + }); + it('dispatches event when an interval filter value is changed', async () => { const fixture = createComponent({ filter: { @@ -65,9 +94,26 @@ describe('filter dialog', () => { rangeInput.componentInstance.rangeValuesChanged.emit({ lowerValue: 7, upperValue: 17, + source: RangeInputSource.TEXT, }); - expect(intervalFilterChangedSpy).toHaveBeenCalled(); + expect(intervalFilterChangedSpy).toHaveBeenCalledOnceWith({ + lowerValue: 7, + upperValue: 17, + source: RangeInputSource.TEXT, + }); + }); + it('dispatches an event when include undefined is changed while viewing an interval filter', async () => { + const fixture = createComponent({ + filter: { + type: DomainType.INTERVAL, + includeUndefined: false, + minValue: 6, + maxValue: 20, + filterLowerValue: 7, + filterUpperValue: 18, + }, + }); const includeUndefinedSpy = spyOn( fixture.componentInstance.includeUndefinedToggled, 'emit' @@ -80,6 +126,25 @@ describe('filter dialog', () => { expect(includeUndefinedSpy).toHaveBeenCalled(); }); + it('renders discrete values', async () => { + createComponent({ + filter: { + type: DomainType.DISCRETE, + includeUndefined: false, + possibleValues: [2, 4, 6, 8], + filterValues: [2, 4, 6], + }, + }); + const checkboxes = await rootLoader.getAllHarnesses( + MatCheckboxHarness.with() + ); + + const checkboxLabels = await Promise.all( + checkboxes.map((checkbox) => checkbox.getLabelText()) + ); + expect(checkboxLabels).toEqual(['2', '4', '6', '8', 'Include Undefined']); + }); + it('dispatches event when an discrete filter value is changed', async () => { const fixture = createComponent({ filter: { @@ -89,19 +154,36 @@ describe('filter dialog', () => { filterValues: [2, 4, 6], }, }); - const discreteFilterChangedSpy = spyOn( - fixture.componentInstance.discreteFilterChanged, - 'emit' + const filterValues: DiscreteFilterValue[] = []; + spyOn(fixture.componentInstance.discreteFilterChanged, 'emit').and.callFake( + (value: DiscreteFilterValue) => filterValues.push(value) ); - const checkbox = await rootLoader.getHarness( + let checkbox = await rootLoader.getHarness( MatCheckboxHarness.with({label: '2'}) ); await checkbox.uncheck(); - fixture.debugElement - .query(By.css('mat-checkbox label')) - .nativeElement.click(); - expect(discreteFilterChangedSpy).toHaveBeenCalled(); + // Unchecking an unchecked box should not trigger an event. + await checkbox.uncheck(); + + await checkbox.check(); + checkbox = await rootLoader.getHarness( + MatCheckboxHarness.with({label: '4'}) + ); + await checkbox.uncheck(); + + expect(filterValues).toEqual([2, 2, 4]); + }); + + it('dispatches an event when include undefined is changed while viewing a discrete filter', async () => { + const fixture = createComponent({ + filter: { + type: DomainType.DISCRETE, + includeUndefined: false, + possibleValues: [2, 4, 6, 8], + filterValues: [2, 4, 6], + }, + }); const includeUndefinedSpy = spyOn( fixture.componentInstance.includeUndefinedToggled, 'emit' @@ -123,11 +205,25 @@ describe('filter dialog', () => { filterValues: ['foo', 'bar', 'baz', 'qaz'], }, }); - expect( - (await rootLoader.getAllHarnesses(MatCheckboxHarness)).length - ).toEqual(5); // 4 options + the include undefined checkbox - - fixture.componentInstance.discreteValueFilter = 'ba'; + const checkboxes = await rootLoader.getAllHarnesses(MatCheckboxHarness); + const checkBoxLabels = await Promise.all( + checkboxes.map((checkbox) => checkbox.getLabelText()) + ); + expect(checkBoxLabels).toEqual([ + 'foo', + 'bar', + 'baz', + 'qaz', + 'Include Undefined', + ]); // 4 options + the include undefined checkbox + + // A mock of a keyboard input event. + const mockEvent = { + target: { + value: 'ba', + }, + }; + fixture.componentInstance.discreteValueKeyUp(mockEvent as any); fixture.detectChanges(); expect( (await rootLoader.getAllHarnesses(MatCheckboxHarness)).length