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/form-field): add error harness #25698

Merged
merged 3 commits into from Sep 26, 2022

Conversation

andrewseguin
Copy link
Contributor

Add an error harness for client tests checking whether there are certain errors on the page. This is very common internally for tests checking validation.

The harness can be retrieved directly or through the form field.

I tried to check the right inheritance structure of other harnesses - might have gotten something mixed up on the way. Would appreciate a review making sure I've got the right inheritance of things. The harness tests passed so that's a good sign.

@andrewseguin andrewseguin force-pushed the error-harness branch 2 times, most recently from 297590b to 69ecbe8 Compare September 25, 2022 18:48
@crisbeto
Copy link
Member

The existing form field harness has getTextErrors and hasErrors methods. Couldn't users depend on them instead? https://github.com/angular/components/blob/main/src/material/form-field/testing/form-field-harness.ts#L124

@devversion
Copy link
Member

ohh, that makes it questionable if it should be a separate harness (like in this PR) or if we should encourage the existing methods. I didn't realize we have these- good point.

@andrewseguin
Copy link
Contributor Author

There's a lot of internal apps that use MatError without using the form field. Whether or not they should is maybe a good thing to discuss, but right now there's no way to get these folks using a test harness to check MatError.

@crisbeto
Copy link
Member

crisbeto commented Sep 26, 2022

I'm not sure if we would get much benefit out of switching those apps to harnesses since MatError basically hasn't changed after we introduced it. Maybe instead of migrating those apps, we can leave TODOs for them?

@andrewseguin
Copy link
Contributor Author

The benefit here is making sure there is consistency with guiding our users to use harnesses with our components. Otherwise we need to special-case "mat-error" in our lint rule, or in our docs, with some reason about why we wouldn't provide one for it.

It would have been better if we failed when mat-error was rendered outside a form field, but with about 500 HTML files in google3, I don't expect ever making that change. One could argue it's a useful directive for user's custom form fields.

@crisbeto
Copy link
Member

Would it make sense to add this harness, but mark it as deprecated so people know to go through the form field one?

@andrewseguin
Copy link
Contributor Author

I'm in favor of deprecating it if we can get users off mat-error in their custom form fields, but that would be a bigger undertaking. I don't think just deprecating the harness is sufficient, you would need to deprecate the use of the mat-error by users outside the form field.

It's really just an element with display: block; color: warn, but people will take what they see https://www.hyrumslaw.com/

@devversion
Copy link
Member

I'm fine going with this approach. Two things/questions:

  1. Should we also have something for mat-hint?
  2. Should we replace the existing methods to rely on the error/hint harnesses?

@andrewseguin
Copy link
Contributor Author

  1. Yeah looks like mat-hint needs it to - There's about 120 HTML files using it outside form field
  2. I think that makes sense - changed the current error APIs to use it in their implementation

@andrewseguin andrewseguin added the action: merge The PR is ready for merge by the caretaker label Sep 26, 2022
@andrewseguin andrewseguin merged commit 36af2a3 into angular:main Sep 26, 2022
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants