feat(material-experimental): add test harness for input #16674

devversion · 2019-08-03T09:02:00Z

See individual commits.

devversion · 2019-08-03T10:38:39Z

It looks like there is one failing test on iOS because the keyup event by sendKeys does not set the event target. This should be fixed once #16645 lands.

Marking as blocked in the meanwhile. This PR can be still reviewed.

jelbourn

LGTM

src/material-experimental/mdc-input/harness/input-harness.ts

devversion · 2019-08-15T10:08:12Z

Addressed feedback. PR is currently still blocked on #16706

mmalerba

This looks good for now, but it'll be interesting to see how it plays with the form-field harness once we create that

mmalerba · 2019-08-15T16:14:18Z

src/material-experimental/mdc-input/harness/input-harness.ts

+  async setValue(newValue: string): Promise<void> {
+    const inputEl = await this.host();
+    await inputEl.clear();
+    // We don't want to send keys for the value if the value is an empty


should we update sendKeys to not do that?

I'm not sure. I could see sendKeys being a noop in that case. Though it also feels like an anti-pattern in general to call sendKeys with an empty value.

Yeah, calling it with empty string is weird and people shouldn't do it, so I'm fine with changing it or just leaving it how it is

I'd vote for leaving it as is, but maybe throwing an error in sendKeys instead? (follow-up)?

googlebot · 2019-08-15T19:33:58Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

mmalerba · 2019-08-23T17:06:01Z

This should be unblocked now that getProperty has landed

googlebot · 2019-08-23T17:54:26Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

devversion · 2019-08-23T17:55:39Z

@mmalerba Rebased. can you have another look?

mmalerba

LGTM with a few nits

src/material-experimental/mdc-input/harness/input-harness.spec.ts

src/material-experimental/mdc-input/harness/input-harness.ts

Adds a test harness for the `MatInput` implementation. Resolves COMP-182

ngbot · 2019-08-23T19:12:24Z

I see that you just added the pr: merge ready label, but the following checks are still failing:
status "ci/circleci: tests_browserstack" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

jelbourn

LGTM

Adds a test harness for the `MatInput` implementation. Resolves COMP-182

angular-automatic-lock-bot · 2019-09-23T00:01:23Z

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.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 3, 2019

devversion marked this pull request as ready for review August 3, 2019 09:22

devversion requested a review from mmalerba as a code owner August 3, 2019 09:22

devversion force-pushed the feat/input-harness branch from 436e5b0 to 8f035ed Compare August 3, 2019 09:22

devversion requested a review from jelbourn as a code owner August 3, 2019 09:22

devversion assigned jelbourn and mmalerba Aug 3, 2019

devversion added pr: merge safe target: patch This PR is targeted for the next patch release merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Aug 3, 2019

devversion added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 3, 2019

devversion force-pushed the feat/input-harness branch from 8f035ed to c8bf616 Compare August 7, 2019 06:43

This comment has been minimized.

Sign in to view

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Aug 7, 2019

devversion force-pushed the feat/input-harness branch 5 times, most recently from 60e36b0 to 82e0f14 Compare August 7, 2019 07:34

devversion removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Aug 7, 2019

jelbourn approved these changes Aug 15, 2019

View reviewed changes

src/material-experimental/mdc-input/harness/input-harness.ts Outdated Show resolved Hide resolved

devversion force-pushed the feat/input-harness branch from 82e0f14 to ce6e2ca Compare August 15, 2019 10:07

mmalerba reviewed Aug 15, 2019

View reviewed changes

mmalerba added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Aug 15, 2019

mmalerba removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 23, 2019

devversion force-pushed the feat/input-harness branch from ce6e2ca to fd5a2ac Compare August 23, 2019 17:54

mmalerba approved these changes Aug 23, 2019

View reviewed changes

mmalerba added the pr: lgtm label Aug 23, 2019

devversion force-pushed the feat/input-harness branch from fd5a2ac to d5a75e2 Compare August 23, 2019 18:13

feat(material-experimental): add test harness for input

5abda4a

Adds a test harness for the `MatInput` implementation. Resolves COMP-182

devversion force-pushed the feat/input-harness branch from d5a75e2 to 5abda4a Compare August 23, 2019 18:21

devversion added the action: merge The PR is ready for merge by the caretaker label Aug 23, 2019

jelbourn approved these changes Aug 23, 2019

View reviewed changes

jelbourn merged commit ab9a847 into angular:master Aug 23, 2019

mmalerba pushed a commit to mmalerba/components that referenced this pull request Aug 27, 2019

feat(material-experimental): add test harness for input (angular#16674)

9090474

Adds a test harness for the `MatInput` implementation. Resolves COMP-182

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 23, 2019

feat(material-experimental): add test harness for input #16674

feat(material-experimental): add test harness for input #16674

Uh oh!

Conversation

devversion commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devversion commented Aug 3, 2019

Uh oh!

This comment has been minimized.

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devversion commented Aug 15, 2019

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

mmalerba Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

devversion Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

mmalerba Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

devversion Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

googlebot commented Aug 15, 2019

Uh oh!

mmalerba commented Aug 23, 2019

Uh oh!

googlebot commented Aug 23, 2019

Uh oh!

devversion commented Aug 23, 2019

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ngbot bot commented Aug 23, 2019

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

devversion commented Aug 3, 2019 •

edited

Loading