Skip to content

Commit

Permalink
make tests more rigorous as per pr comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@rileyajones
rileyajones committed Aug 14, 2023
1 parent 16105dc commit 1e47e93
Show file tree
Hide file tree
Showing 3 changed files with 136 additions and 38 deletions.
47 changes: 24 additions & 23 deletions tensorboard/defs/defs.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -91,7 +90,6 @@ def tf_js_binary(
**kwargs
)


def tf_ng_prod_js_binary(
name,
compile,
Expand All @@ -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
Expand All @@ -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):
Expand All @@ -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,
Expand All @@ -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.
Expand Down Expand Up @@ -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,
],
)

Expand All @@ -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 + [
Expand Down
1 change: 1 addition & 0 deletions tensorboard/webapp/widgets/data_table/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
126 changes: 111 additions & 15 deletions tensorboard/webapp/widgets/data_table/filter_dialog_test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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: {
Expand All @@ -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'
Expand All @@ -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: {
Expand All @@ -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'
Expand All @@ -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
Expand Down

0 comments on commit 1e47e93

Please sign in to comment.