Skip to content

fix(ivy): adding TestBed.overrideProvider support - #27693

Closed
AndrewKushnir wants to merge 2 commits into
angular:masterfrom
AndrewKushnir:FW-788_test_bed_override_providers_v2
Closed

fix(ivy): adding TestBed.overrideProvider support#27693
AndrewKushnir wants to merge 2 commits into
angular:masterfrom
AndrewKushnir:FW-788_test_bed_override_providers_v2

Conversation

@AndrewKushnir

@AndrewKushnir AndrewKushnir commented Dec 15, 2018

Copy link
Copy Markdown
Contributor

Prior to this change, provider overrides defined via TestBed.overrideProvider were not applied to Components/Directives. Now providers are taken into account while compiling Components/Directives (metadata is updated accordingly before being passed to compilation).

This PR resolves FW-788.

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Dec 15, 2018
@AndrewKushnir
AndrewKushnir requested review from alxhub and kara December 15, 2018 22:51
@ngbot ngbot Bot added this to the needsTriage milestone Dec 15, 2018
@AndrewKushnir AndrewKushnir mentioned this pull request Dec 15, 2018
Closed
2 of 5 tasks
@mary-poppins

mary-poppins commented Dec 15, 2018

Copy link
Copy Markdown

You can preview 673e290 at https://pr27693-673e290.ngbuilds.io/.

@mary-poppins

mary-poppins commented Dec 17, 2018

Copy link
Copy Markdown

You can preview 0bd7dfa at https://pr27693-0bd7dfa.ngbuilds.io/.

@mary-poppins

mary-poppins commented Dec 17, 2018

Copy link
Copy Markdown

You can preview d7827c8 at https://pr27693-d7827c8.ngbuilds.io/.

@mary-poppins

mary-poppins commented Dec 17, 2018

Copy link
Copy Markdown

You can preview 0752248 at https://pr27693-0752248.ngbuilds.io/.

@AndrewKushnir AndrewKushnir mentioned this pull request Dec 17, 2018
Closed
2 of 5 tasks
kara
kara approved these changes Dec 18, 2018
View reviewed changes

@kara kara left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, though curious about forward refs

alxhub
alxhub previously requested changes Dec 19, 2018
View reviewed changes
Comment thread packages/core/testing/src/r3_test_bed.ts Outdated
Comment thread packages/core/testing/src/r3_test_bed.ts Outdated
Comment thread packages/core/testing/src/r3_test_bed.ts Outdated
@AndrewKushnir
AndrewKushnir force-pushed the FW-788_test_bed_override_providers_v2 branch from 0752248 to 09b8832 Compare December 19, 2018 23:36
@mary-poppins

mary-poppins commented Dec 19, 2018

Copy link
Copy Markdown

You can preview 09b8832 at https://pr27693-09b8832.ngbuilds.io/.

@AndrewKushnir
AndrewKushnir force-pushed the FW-788_test_bed_override_providers_v2 branch from 09b8832 to b133278 Compare December 19, 2018 23:52
@mary-poppins

mary-poppins commented Dec 19, 2018

Copy link
Copy Markdown

You can preview b133278 at https://pr27693-b133278.ngbuilds.io/.

@AndrewKushnir

AndrewKushnir commented Dec 20, 2018

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @alxhub! I've updated provider override logic to use Token<->Provider[] Map and take into account the fact that providers array can be nested. Here is my fixup commit: 0b72ae3.
Could you please take another look?
Thank you.

@mary-poppins

mary-poppins commented Dec 20, 2018

Copy link
Copy Markdown

You can preview 0b72ae3 at https://pr27693-0b72ae3.ngbuilds.io/.

@AndrewKushnir

AndrewKushnir commented Dec 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Presubmit

kara
kara approved these changes Dec 21, 2018
View reviewed changes

@kara kara left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@kara
kara dismissed alxhub’s stale review December 21, 2018 01:29

Changes are resolved and he is OOO for a few weeks. If there are further comments, we can land in a follow-up PR.

@kara kara added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 21, 2018
@AndrewKushnir AndrewKushnir mentioned this pull request Dec 21, 2018
Closed
2 of 5 tasks
@AndrewKushnir
fix(ivy): adding TestBed.overrideProvider support
d9b17da 
Prior to this change, provider overrides defined via TestBed.overrideProvider were not applied to Components/Directives. Now providers are taken into account while compiling Components/Directives (metadata is updated accordingly before being passed to compilation).
@AndrewKushnir
AndrewKushnir force-pushed the FW-788_test_bed_override_providers_v2 branch from 0b72ae3 to 04b1ffd Compare December 21, 2018 20:20
@mary-poppins

mary-poppins commented Dec 21, 2018

Copy link
Copy Markdown

You can preview 04b1ffd at https://pr27693-04b1ffd.ngbuilds.io/.

@matsko matsko closed this in 4b67b0a Dec 21, 2018
matsko added a commit to matsko/angular that referenced this pull request Dec 26, 2018
@matsko
revert: fix(ivy): adding TestBed.overrideProvider support (angular#27693
7f479c6 
)

This reverts commit 4b67b0a.
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 14, 2019

Copy link
Copy Markdown

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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alxhub alxhub alxhub left review comments
+2 more reviewers
@IgorMinar IgorMinar IgorMinar approved these changes
@kara kara kara approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release type: bug/fix

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

6 participants

@AndrewKushnir @mary-poppins @IgorMinar @kara @alxhub @googlebot