Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(material/list): add test harnesses for list components. #17859

Merged
merged 7 commits into from Dec 4, 2019

Conversation

@mmalerba
Copy link
Contributor

mmalerba commented Dec 2, 2019

No description provided.

@mmalerba mmalerba requested review from jelbourn and crisbeto Dec 2, 2019
@mmalerba mmalerba requested a review from devversion as a code owner Dec 2, 2019
@googlebot googlebot added the cla: yes label Dec 2, 2019
@mmalerba mmalerba force-pushed the mmalerba:list-harness branch from d4476f5 to 2288e7f Dec 3, 2019
@mmalerba mmalerba requested a review from angular/dev-infra-components as a code owner Dec 3, 2019
src/cdk/testing/component-harness.ts Outdated Show resolved Hide resolved
src/material/list/testing/list-harness.ts Outdated Show resolved Hide resolved
src/material/list/testing/list-harness.ts Outdated Show resolved Hide resolved
text?: string | RegExp;
}

export interface ListItemHarnessFilters extends BaseListItemHarnessFilters {}

This comment has been minimized.

Copy link
@jelbourn

jelbourn Dec 3, 2019

Member

Any reason not to just have ListItemHarnessFilters be the base type instead of having a separate BaseListItemHarnessFilters type?

This comment has been minimized.

Copy link
@mmalerba

mmalerba Dec 3, 2019

Author Contributor

It didn't feel right since the other types don't actually extend MatList. However, I don't really see needing to add a filter for normal list items that doesn't also apply to the other list item types, so it would probably be fine if we want to do that.

src/material/list/testing/list-harness.ts Outdated Show resolved Hide resolved
src/material/divider/testing/shared.spec.ts Outdated Show resolved Hide resolved
src/material/list/testing/list-harness.ts Outdated Show resolved Hide resolved
src/material/list/testing/list-item-harness.ts Outdated Show resolved Hide resolved
src/material/list/testing/list-item-harness.ts Outdated Show resolved Hide resolved
@mmalerba mmalerba force-pushed the mmalerba:list-harness branch from 2288e7f to 81cd497 Dec 3, 2019
Copy link
Contributor Author

mmalerba left a comment

Most of the comments addressed, will move the files around in a followup commit to make following the comments easier.

export interface SelectionListHarnessFilters extends BaseHarnessFilters {}

export interface BaseListItemHarnessFilters extends BaseHarnessFilters {
text?: string | RegExp;

This comment has been minimized.

Copy link
@mmalerba

mmalerba Dec 3, 2019

Author Contributor

I don't think so. Because the list doesn't have hierarchical sections, its not always clear if an item belongs to a header or not. e.g.:

Subheader 1
  Item 1
  Item 2
-----------
  Item 3 <-- under Subheader 1?
text?: string | RegExp;
}

export interface ListItemHarnessFilters extends BaseListItemHarnessFilters {}

This comment has been minimized.

Copy link
@mmalerba

mmalerba Dec 3, 2019

Author Contributor

It didn't feel right since the other types don't actually extend MatList. However, I don't really see needing to add a filter for normal list items that doesn't also apply to the other list item types, so it would probably be fine if we want to do that.

src/material/list/testing/list-harness.ts Outdated Show resolved Hide resolved
src/material/list/testing/list-harness.ts Outdated Show resolved Hide resolved
@mmalerba mmalerba force-pushed the mmalerba:list-harness branch from 81cd497 to cbbadda Dec 3, 2019
@@ -15,12 +15,14 @@ entryPoints = [
"dialog",
"dialog/testing",
"divider",
"divider/testing",

This comment has been minimized.

Copy link
@crisbeto

crisbeto Dec 3, 2019

Member

It's consistent, but it feels a little silly to have a test harness for what is basically a div.

This comment has been minimized.

Copy link
@mmalerba

mmalerba Dec 3, 2019

Author Contributor

Yeah its a little silly, but there are actually couple methods on it, and just having the selector encapsulated in a harness is kind of nice

src/material/list/testing/list-harness.ts Outdated Show resolved Hide resolved

/** Harness for interacting with a list subheader. */
export class MatSubheaderHarness extends ComponentHarness {
static hostSelector = '[mat-subheader], [matSubheader]';

This comment has been minimized.

Copy link
@crisbeto

crisbeto Dec 3, 2019

Member

Aren't we running the risk of these selectors going out of sync? Maybe in a follow-up we should pull them out into variables?

This comment has been minimized.

Copy link
@mmalerba

mmalerba Dec 3, 2019

Author Contributor

I think it'll be ok, usually when I remove a selector I do a global search for e.g. 'mat-subheader' to find and fix them all, that should catch this.

src/material/list/testing/list-item-harness.ts Outdated Show resolved Hide resolved
mmalerba added 5 commits Nov 22, 2019
@mmalerba mmalerba force-pushed the mmalerba:list-harness branch from b6b9804 to 88ce469 Dec 3, 2019
@mmalerba

This comment has been minimized.

Copy link
Contributor Author

mmalerba commented Dec 3, 2019

Comments addressed, PTAL

@mmalerba mmalerba force-pushed the mmalerba:list-harness branch from 88ce469 to 2a6c91a Dec 3, 2019
Copy link
Member

jelbourn left a comment

LGTM

src/material/list/testing/list-harness-base.ts Outdated Show resolved Hide resolved
Copy link
Member

crisbeto left a comment

LGTM

@jelbourn jelbourn merged commit 49b6dbd into angular:master Dec 4, 2019
11 checks passed
11 checks passed
ci/angular: merge status All checks passed!
ci/circleci: api_golden_checks Your tests passed on CircleCI!
Details
ci/circleci: bazel_build Your tests passed on CircleCI!
Details
ci/circleci: build_release_packages Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: tests_browserstack Your tests passed on CircleCI!
Details
ci/circleci: tests_local_browsers Your tests passed on CircleCI!
Details
ci/circleci: tests_saucelabs Your tests passed on CircleCI!
Details
ci/circleci: view_engine_test Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.