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

test(material-experimental/chips) Port unit tests for mdc chips #16447

Merged
merged 1 commit into from
Jul 12, 2019

Conversation

vanessanschmitt
Copy link
Collaborator

@vanessanschmitt vanessanschmitt commented Jul 4, 2019

Ported existing unit tests as follows:

  • Copied chip-remove.spec.
  • Copied chip-input.spec and changed its test component template to use grid/row.
  • Copied chip-list.spec basic tests to chip-set.spec.
  • Copied chip-list.spec to chip-listbox.spec and removed form-field integration, mat-chip-remove integration, and related keyboard shortcuts.
  • Copied chip-list.spec to chip-grid.spec and removed the selection behavior. Updated keyboard tests to reflect that it is using GridFocusKeyManager, which has both an active row and an active column. Updated some of the form tests where the value was being changed via selection to change the value in a different way, like by adding a chip.
  • Copied chip.spec basic tests to chip.spec.
  • Copied chip.spec to chip-option.spec and removed logic related to user removal of the chip.
  • Copied chip.spec to chip-row.spec and removed selection logic.

Also updated component code to fix bugs/missing features that were caught by the unit tests:

  • Added missing ControlValueAccessor method setDisabledState to mat-chip-listbox and mat-chip-grid
  • Stopped overwriting user-defined tab index in mat-chip-listbox and mat-chip-grid.
  • Removed role from empty mat-chip-listbox and mat-chip-grid.
  • Added a new subscription in mat-chip-set to chip destroyed events, and moved the logic that updates the lastDestroyedChipIndex out of the removed subscription into the new destroyed subscription.
  • Replaced _optionChip and _rowChips ViewChildren in mat-chip-listbox and mat-chip-grid with just overriding _chips, to fix bugs where the _chips.changes subscription was trying to do things with _optionChips/_rowChips but those QueryLists were still out of date.
  • Added placeholder setter to mat-chip-grid.
  • Added value setter to mat-chip-listbox.
  • Added injected animationMode to mat-chip and its subcomponents.
  • Updated rippleConfig to be injected into mat-chip and its subcomponents.
  • Switched click handler to mousedown handler in mat-chip-row.
  • Added mat-chip-remove class to remove icon.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 4, 2019
@vanessanschmitt vanessanschmitt force-pushed the unit-tests branch 3 times, most recently from 36d8252 to c1c3dcb Compare July 8, 2019 19:34
@vanessanschmitt
Copy link
Collaborator Author

@jelbourn

@vanessanschmitt vanessanschmitt force-pushed the unit-tests branch 11 times, most recently from 6a92696 to 165d853 Compare July 9, 2019 00:41
@jelbourn
Copy link
Member

jelbourn commented Jul 9, 2019

Could you expand the commit message to talk a bit about any changes that you had to make? Doing some cursory diffs, it mostly seems like:

  • Copied chip-list.spec to chip-grid.spec and removed the selection behavior and removed keyboard shortcuts
  • Copied chip-list.spec to chip-listbox.spec and removed form-field integration and related keyboard shortcuts
  • Copied chip.spec to chip-option.spec and ...

@vanessanschmitt vanessanschmitt force-pushed the unit-tests branch 3 times, most recently from d865572 to c00af77 Compare July 9, 2019 20:58
@vanessanschmitt
Copy link
Collaborator Author

@jelbourn Commit message updated!

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jul 10, 2019
@ngbot
Copy link

ngbot bot commented Jul 10, 2019

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure 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.

@vanessanschmitt vanessanschmitt force-pushed the unit-tests branch 2 times, most recently from 7d91ffe to 3ada584 Compare July 10, 2019 17:49
@vanessanschmitt
Copy link
Collaborator Author

related to #16173

@jelbourn
Copy link
Member

@vanessanschmitt could you rebase the PR? Another change went in that updated all of the BUILD files in the repo for a new version of bazel's nodejs rules (which was somewhat breaking)

@vanessanschmitt vanessanschmitt force-pushed the unit-tests branch 2 times, most recently from 4f4970e to d8d10af Compare July 12, 2019 01:19
@vanessanschmitt
Copy link
Collaborator Author

vanessanschmitt commented Jul 12, 2019

@jelbourn Rebased!

Ported existing unit tests as follows:
- Copied chip-remove.spec.
- Copied chip-input.spec and changed its test component template to use
grid/row.
- Copied chip-list.spec basic tests to chip-set.spec.
- Copied chip-list.spec to chip-listbox.spec and removed form-field
integration, mat-chip-remove integration, and related keyboard shortcuts.
- Copied chip-list.spec to chip-grid.spec and removed the selection behavior.
Updated keyboard tests to reflect that it is using GridFocusKeyManager,
which has both an active row and an active column. Updated some of the form
tests where the value was being changed via selection to change the value
in a different way, like by adding a chip.
- Copied chip.spec basic tests to chip.spec.
- Copied chip.spec to chip-option.spec and removed logic related to user
removal of the chip.
- Copied chip.spec to chip-row.spec and removed selection logic.

Also updated component code to fix bugs/missing features that were caught by
the unit tests:
- Added missing ControlValueAccessor method setDisabledState to mat-chip-listbox
and mat-chip-grid
- Stopped overwriting  user-defined tab index to mat-chip-listbox and mat-chip-grid.
- Removed role from empty mat-chip-listbox and mat-chip-grid.
- Added a new subscription in mat-chip-set to chip destroyed events, and moved the
logic that updates the lastDestroyedChipIndex out of the removed subscription into
the new destroyed subscription.
- Replaced _optionChip and _rowChips ViewChildren in mat-chip-listbox and
mat-chip-grid with just overriding _chips, to fix bugs where the
_chips.changes subscription was trying to do things with
_optionChips/_rowChips but those QueryLists were still out of date.
- Added placeholder setter to mat-chip-grid.
- Added value setter to mat-chip-listbox.
- Added injected animationMode to mat-chip and its subcomponents.
- Updated rippleConfig to be injected into mat-chip and its subcomponents.
- Switched click handler to mousedown handler in mat-chip-row.
- Added mat-chip-remove class to remove icon.
@jelbourn jelbourn merged commit dcde115 into angular:master Jul 12, 2019
@vanessanschmitt vanessanschmitt deleted the unit-tests branch July 12, 2019 17:41
@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 Sep 11, 2019
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 cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants