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/icon): New NoopIconModule for silencing unit test errors #18151

Merged
merged 1 commit into from Jan 13, 2020

Conversation

@osuritz
Copy link
Contributor

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 Jan 10, 2020
@googlebot googlebot added the cla: yes label Jan 10, 2020
@osuritz osuritz force-pushed the osuritz:noop_icon_registry branch 2 times, most recently from fbc814b to 9a7f647 Jan 10, 2020
@jelbourn jelbourn requested a review from andrewseguin Jan 11, 2020
@osuritz osuritz force-pushed the osuritz:noop_icon_registry branch 3 times, most recently from e93927e to 800041a Jan 11, 2020
@osuritz osuritz force-pushed the osuritz:noop_icon_registry branch 4 times, most recently from 0e96402 to a0be7b2 Jan 11, 2020
@osuritz osuritz requested a review from angular/dev-infra-components as a code owner Jan 13, 2020
Copy link
Member

crisbeto left a comment

LGTM, aside from a couple of nits.

return this;
}

ngOnDestroy() { }

This comment has been minimized.

Copy link
@crisbeto

crisbeto Jan 13, 2020

Member

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

This comment has been minimized.

Copy link
@osuritz

osuritz Jan 13, 2020

Author Contributor

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.

src/material/icon/testing/noop-icon-registry.ts Outdated Show resolved Hide resolved
@osuritz osuritz force-pushed the osuritz:noop_icon_registry branch from a0be7b2 to 5db673e Jan 13, 2020
Copy link
Contributor Author

osuritz left a comment

Done!

@osuritz osuritz force-pushed the osuritz:noop_icon_registry branch from 5db673e to 29398dd Jan 13, 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 force-pushed the osuritz:noop_icon_registry branch from 29398dd to 304e126 Jan 13, 2020
Copy link
Member

jelbourn left a comment

LGTM

@jelbourn jelbourn added this to the 9.1.0 milestone Jan 13, 2020
@jelbourn

This comment has been minimized.

Copy link
Member

jelbourn commented Jan 13, 2020

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

@osuritz

This comment has been minimized.

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
12 checks passed
12 checks passed
ci/angular: merge status All checks passed!
ci/circleci: api_golden_checks Your tests passed on CircleCI!
Details
ci/circleci: bazel_build Your tests passed on CircleCI!
Details
ci/circleci: build_release_packages Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: ngcc_compatibility Your tests passed on CircleCI!
Details
ci/circleci: tests_browserstack Your tests passed on CircleCI!
Details
ci/circleci: tests_local_browsers Your tests passed on CircleCI!
Details
ci/circleci: tests_saucelabs Your tests passed on CircleCI!
Details
ci/circleci: view_engine_test Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
@osuritz osuritz deleted the osuritz:noop_icon_registry branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.