Skip to content

feat(material/icon): New NoopIconModule for silencing unit test errors #18151

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

Jump to bottom
Merged
merged 1 commit into from
Jan 13, 2020
Merged

feat(material/icon): New NoopIconModule for silencing unit test errors #18151

merged 1 commit into from
Jan 13, 2020

Conversation

osuritz
Copy link
Contributor

@osuritz osuritz commented Jan 10, 2020

Introducing the NoopIconModule to help silence Karma tests throwing warnings about custom icons not found. This added a lot of noise to tests output and it's now gone when NoopMatIconModule is installed in tests. The actual error this silences is Error retrieving icon :xyz! Unable to find icon with the name ":xyz"

@osuritz osuritz requested a review from jelbourn as a code owner January 10, 2020 23:30
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 10, 2020
@osuritz osuritz force-pushed the noop_icon_registry branch 2 times, most recently from fbc814b to 9a7f647 Compare January 10, 2020 23:40
@jelbourn jelbourn added G This is is related to a Google internal issue feature This issue represents a new feature or feature request rather than a bug or bug fix P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Jan 11, 2020
@jelbourn jelbourn requested a review from andrewseguin January 11, 2020 00:06
@osuritz osuritz force-pushed the noop_icon_registry branch 3 times, most recently from e93927e to 800041a Compare January 11, 2020 00:43
jelbourn
jelbourn reviewed Jan 11, 2020
View reviewed changes
@osuritz osuritz force-pushed the noop_icon_registry branch 4 times, most recently from 0e96402 to a0be7b2 Compare January 13, 2020 18:34
@osuritz osuritz requested a review from a team as a code owner January 13, 2020 18:34
crisbeto
crisbeto approved these changes Jan 13, 2020
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM, aside from a couple of nits.

src/material/icon/testing/noop-icon-registry.ts Outdated
return this;
}

ngOnDestroy() { }
Copy link
Member

Choose a reason for hiding this comment

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

Can we drop this one or is it required by the PublicApi implementation? Technically the lifecycle hooks aren't part of the public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tl;dr: It's required.

This one is unfortunate: it's required because PublicApi. And then it turn the linter will complains if you have a lifecycle method without implementing the related interface, so it does add cruft. But, from a pure OO standpoint (i.e. Liskov substitution principle) it's more correct to support all the public methods that the original class exposes.

@osuritz osuritz force-pushed the noop_icon_registry branch from a0be7b2 to 5db673e Compare January 13, 2020 19:07
osuritz
osuritz commented Jan 13, 2020
View reviewed changes
Copy link
Contributor Author

@osuritz osuritz left a comment

Choose a reason for hiding this comment

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

Done!

@osuritz osuritz force-pushed the noop_icon_registry branch from 5db673e to 29398dd Compare January 13, 2020 19:15
@osuritz
feat(material/icon): New NoopIconModule for silencing unit test errors
304e126 
Introducing the NoopIconModule to help silence Karma tests throwing warnings about custom icons not found. This added a lot of noise to tests output and it's now gone when `NoopMatIconModule` is installed in tests. The actual error this silences is `Error retrieving icon :xyz! Unable to find icon with the name ":xyz"`
@osuritz osuritz force-pushed the noop_icon_registry branch from 29398dd to 304e126 Compare January 13, 2020 22:53
jelbourn
jelbourn approved these changes Jan 13, 2020
View reviewed changes
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 target: minor This PR is targeted for the next minor release label Jan 13, 2020
@jelbourn jelbourn added this to the 9.1.0 milestone Jan 13, 2020
@jelbourn
Copy link
Member

@osuritz interested in adding docs in a follow-up PR?

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jan 13, 2020
@osuritz
Copy link
Contributor Author

osuritz commented Jan 13, 2020

Renamed from NoopMatIconRegistry to FakeMatIconRegistry as per @jelbourn's team wishes.

@jelbourn jelbourn merged commit 71955d2 into angular:master Jan 13, 2020
@osuritz osuritz deleted the noop_icon_registry branch January 14, 2020 00:11
yifange pushed a commit to yifange/components that referenced this pull request Jan 30, 2020
@osuritz @yifange
feat(material/icon): New FakeMatIconRegitry for unit tests (angular#1…
253f9e9 
…8151)

Adds `MatIconTestingMdoule` to eliminate warnings of the form `Error retrieving icon :xyz! Unable to find icon with the name ":xyz"`. This warning added noise to tests output. When `MaticonTestingModule` is installed in tests, the fake implementation of the icon registry will always return fake SVG icons.
@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 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@crisbeto crisbeto crisbeto approved these changes

@devversion devversion devversion approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement feature This issue represents a new feature or feature request rather than a bug or bug fix G This is is related to a Google internal issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
9.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@osuritz @jelbourn @crisbeto @devversion @googlebot