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

fix(material-experimental/mdc-radio): de-duplicate test harness logic #21532

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

crisbeto
Copy link
Member

Changes the MDC-based radio harnesses to extend the base ones directly since the logic is identical, aside from some selectors.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Jan 10, 2021
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 10, 2021
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 the action: merge The PR is ready for merge by the caretaker label Jan 11, 2021
@mmalerba
Copy link
Contributor

mmalerba commented Jan 16, 2021

I'm seeing the following error from this (and some of the other PRs to share harness code):

Type 'import("material/radio/testing/radio-harness").MatRadioButtonHarness[]' is not assignable to type 'import("material-experimental/mdc-radio/testing/radio-harness").MatRadioButtonHarness[]'.
  Type 'import("material/radio/testing/radio-harness").MatRadioButtonHarness' is not assignable to type 'import("material-experimental/mdc-radio/testing/radio-harness").MatRadioButtonHarness'.
    Property '_textLabel' is protected but type 'MatRadioButtonHarness' is not a class derived from 'MatRadioButtonHarness'.

The code that causes it looks like this:

import {MatRadioButtonHarness, MatRadioGroupHarness} from '@angular/material/mdc-radio/testing';

...

let radioGroupHarness: MatRadioGroupHarness[];
let radioButtonRelations: MatRadioButtonHarness[];

...

radioGroupHarness =
    await harnessLoader.getAllHarnesses(MatRadioGroupHarness);
radioButtonRelations = await radioGroupHarness[0].getRadioButtons();

@crisbeto
Copy link
Member Author

It's odd that our own build didn't complain about it. Maybe we can flip on a compiler flag to make it behave closer to g3? Also is it possible that they're importing both the non-MDC and MDC harnesses and they're using one to type the variable and the other one to look up the elements?

@mmalerba
Copy link
Contributor

That's what I assumed was happening at first, but no - they're not importing anything from radio/testing and the variables are explicitly typed with the class imported from mdc-radio/testing like I showed above.

@mmalerba
Copy link
Contributor

Maybe it has something to do with the with function? this looks a little suspect:

return new HarnessPredicate<NonMdcMatRadioButtonHarness>(MatRadioButtonHarness, options)

@crisbeto
Copy link
Member Author

I see, I managed to reproduce it locally too. It's only the second call that is causing the compilation error. I think that it'll have to use the same approach where we have a base class for the harnesses. I'll rework it and let you know when it's done.

Changes the MDC-based radio harnesses to extend the base ones directly since the logic
is identical, aside from some selectors.
@crisbeto
Copy link
Member Author

Reworked the PR to avoid the compilation error.

@andrewseguin
Copy link
Contributor

Passes TGP - will be merged on the next round

@andrewseguin andrewseguin merged commit 9d92927 into angular:master Jan 25, 2021
andrewseguin pushed a commit that referenced this pull request Jan 25, 2021
…#21532)

Changes the MDC-based radio harnesses to extend the base ones directly since the logic
is identical, aside from some selectors.

(cherry picked from commit 9d92927)
wagnermaciel pushed a commit to wagnermaciel/components that referenced this pull request Jan 26, 2021
…angular#21532)

Changes the MDC-based radio harnesses to extend the base ones directly since the logic
is identical, aside from some selectors.
mmalerba pushed a commit to mmalerba/components that referenced this pull request Jan 29, 2021
…angular#21532)

Changes the MDC-based radio harnesses to extend the base ones directly since the logic
is identical, aside from some selectors.
wagnermaciel pushed a commit to wagnermaciel/components that referenced this pull request Feb 8, 2021
…angular#21532)

Changes the MDC-based radio harnesses to extend the base ones directly since the logic
is identical, aside from some selectors.
wagnermaciel pushed a commit to wagnermaciel/components that referenced this pull request Feb 8, 2021
…angular#21532)

Changes the MDC-based radio harnesses to extend the base ones directly since the logic
is identical, aside from some selectors.
@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 Feb 25, 2021
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 P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants