Skip to content

refactor(form-field/testing): deprecate methods that return TestElement #19940

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 11, 2020
Merged

refactor(form-field/testing): deprecate methods that return TestElement #19940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/material-experimental/mdc-form-field/testing/form-field-harness.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,19 +160,35 @@ export class MatFormFieldHarness extends ComponentHarness {
/**
* Gets a reference to the container element which contains all projected
* prefixes of the form-field.
* @deprecated Use `getPrefixText` instead.
* @breaking-change 11.0.0
*/
async getHarnessLoaderForPrefix(): Promise<TestElement|null> {
return this._prefixContainer();
}

/** Gets the text inside the prefix element. */
async getPrefixText(): Promise<string> {
const prefix = await this._prefixContainer();
return prefix ? prefix.text() : '';
}

/**
* Gets a reference to the container element which contains all projected
* suffixes of the form-field.
* @deprecated Use `getSuffixText` instead.
* @breaking-change 11.0.0
*/
async getHarnessLoaderForSuffix(): Promise<TestElement|null> {
return this._suffixContainer();
}

/** Gets the text inside the suffix element. */
async getSuffixText(): Promise<string> {
const suffix = await this._suffixContainer();
return suffix ? suffix.text() : '';
}

/**
* Whether the form control has been touched. Returns "null"
* if no form control is set up.
Expand Down
16 changes: 16 additions & 0 deletions src/material/form-field/testing/form-field-harness.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,19 +165,35 @@ export class MatFormFieldHarness extends ComponentHarness {
/**
* Gets a reference to the container element which contains all projected
* prefixes of the form-field.
* @deprecated Use `getPrefixText` instead.
* @breaking-change 11.0.0
*/
async getHarnessLoaderForPrefix(): Promise<TestElement|null> {
return this._prefixContainer();
}

/** Gets the text inside the prefix element. */
async getPrefixText(): Promise<string> {
const prefix = await this._prefixContainer();
return prefix ? prefix.text() : '';
}

/**
* Gets a reference to the container element which contains all projected
* suffixes of the form-field.
* @deprecated Use `getSuffixText` instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we actually just fix this? Seems like this happened by mistake, but the methods actually should exist?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I think we didn't do <..>Text is that a prefix/suffix doesn't necessarily need to be about text. i.e. it can contain icons or arbitrary content, which can be tested against with the harness loader too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, looks like this has been changed in 1c1af58 as part of Miles' suggestion (#17874 (comment)) but the logic has not been updated properly. Any reason to deprecate the actual method instead of making it return a harness loader?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to add the text method since I don't see what else you would be doing with the TestElement. We could make it return a harness loader, but at this point it'll be a breaking change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah for sure, it shouldn't be a TestElement. It just feels wrong to me to deprecate the method while we actually would want to keep the method (just with the fix IMO). cc. @mmalerba for thoughts on this.

The additional methods you added are a nice addition we could keep anyway I guess.

Copy link
Member

@devversion devversion Jul 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crisbeto Chatted with Miles. Looks like we can deprecate this as we'll add the more generic harness loader methods w/ an enum for accessing the prefix/suffix containers. That makes sense to me.

* @breaking-change 11.0.0
*/
async getHarnessLoaderForSuffix(): Promise<TestElement|null> {
return this._suffixContainer();
}

/** Gets the text inside the suffix element. */
async getSuffixText(): Promise<string> {
const suffix = await this._suffixContainer();
return suffix ? suffix.text() : '';
}

/**
* Whether the form control has been touched. Returns "null"
* if no form control is set up.
Expand Down
16 changes: 6 additions & 10 deletions src/material/form-field/testing/shared.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,20 +188,16 @@ export function runHarnessTests(
expect(await formFields[1].getTextHints()).toEqual([]);
});

it('should be able to get prefix container of form-field', async () => {
it('should be able to get the prefix text of a form-field', async () => {
const formFields = await loader.getAllHarnesses(formFieldHarness);
const prefixContainers = await Promise.all(formFields.map(f => f.getHarnessLoaderForPrefix()));
expect(prefixContainers[0]).not.toBe(null);
expect(await prefixContainers[0]!.text()).toBe('prefix_textprefix_text_2');
expect(prefixContainers[1]).toBe(null);
const prefixTexts = await Promise.all(formFields.map(f => f.getPrefixText()));
expect(prefixTexts).toEqual(['prefix_textprefix_text_2', '', '', '', '']);
});

it('should be able to get suffix container of form-field', async () => {
it('should be able to get the suffix text of a form-field', async () => {
const formFields = await loader.getAllHarnesses(formFieldHarness);
const suffixContainer = await Promise.all(formFields.map(f => f.getHarnessLoaderForSuffix()));
expect(suffixContainer[0]).not.toBe(null);
expect(await suffixContainer[0]!.text()).toBe('suffix_text');
expect(suffixContainer[1]).toBe(null);
const suffixTexts = await Promise.all(formFields.map(f => f.getSuffixText()));
expect(suffixTexts).toEqual(['suffix_text', '', '', '', '']);
});

it('should be able to check if form field has been touched', async () => {
Expand Down
2 changes: 2 additions & 0 deletions tools/public_api_guard/material/form-field/testing.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ export declare class MatFormFieldHarness extends ComponentHarness {
getHarnessLoaderForPrefix(): Promise<TestElement | null>;
getHarnessLoaderForSuffix(): Promise<TestElement | null>;
getLabel(): Promise<string | null>;
getPrefixText(): Promise<string>;
getSuffixText(): Promise<string>;
getTextErrors(): Promise<string[]>;
getTextHints(): Promise<string[]>;
getThemeColor(): Promise<'primary' | 'accent' | 'warn'>;
Expand Down