Skip to content

feat(test): add withProviders for per test providers#5891

Closed
juliemr wants to merge 1 commit into
angular:masterfrom
juliemr:per-test-spec
Closed

feat(test): add withProviders for per test providers#5891
juliemr wants to merge 1 commit into
angular:masterfrom
juliemr:per-test-spec

Conversation

@juliemr
Copy link
Copy Markdown
Member

@juliemr juliemr commented Dec 15, 2015

Closes #5128

@juliemr juliemr added area: testing Issues related to Angular testing features, such as TestBed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 15, 2015
@IgorMinar IgorMinar added this to the beta.1 milestone Dec 15, 2015
@IgorMinar
Copy link
Copy Markdown
Contributor

IgorMinar commented Dec 15, 2015

please rebase

@IgorMinar IgorMinar assigned IgorMinar and juliemr and unassigned IgorMinar Dec 15, 2015
@IgorMinar IgorMinar added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 15, 2015
@juliemr
Copy link
Copy Markdown
Member Author

juliemr commented Dec 15, 2015

This is pending my fixing and resubmitting #5819

I'll update when it's ready to go.

@IgorMinar IgorMinar modified the milestone: beta.1 Dec 15, 2015
@juliemr juliemr removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews state: blocked labels Dec 18, 2015
@juliemr
Copy link
Copy Markdown
Member Author

juliemr commented Dec 18, 2015

This is now unblocked and rebased.

@juliemr juliemr assigned IgorMinar and unassigned juliemr Dec 18, 2015
@juliemr juliemr force-pushed the per-test-spec branch 2 times, most recently from 8f5a717 to 295e6e7 Compare December 28, 2015 23:10
@juliemr
Copy link
Copy Markdown
Member Author

juliemr commented Dec 29, 2015

This is now actually passing Travis - typing issues fixed.

@juliemr
Copy link
Copy Markdown
Member Author

juliemr commented Jan 13, 2016

@IgorMinar could you take another look at this? All rebased and passing.

@jelbourn
Copy link
Copy Markdown
Contributor

jelbourn commented Feb 1, 2016

Prodding @IgorMinar

@juliemr
Copy link
Copy Markdown
Member Author

juliemr commented Feb 9, 2016

Further prodding @IgorMinar

Is anyone available to review?

@juliemr
Copy link
Copy Markdown
Member Author

juliemr commented Feb 17, 2016

@btford would you be able to review?

@juliemr juliemr assigned btford and unassigned IgorMinar Feb 17, 2016
kegluneq
kegluneq reviewed Feb 17, 2016
View reviewed changes
Comment thread modules/angular2/src/testing/test_injector.ts Outdated
Copy link
Copy Markdown

@kegluneq kegluneq Feb 17, 2016

Choose a reason for hiding this comment

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

opinion nit: If we're worried about perf here, I think this check fits better in addProviders, but up to you whether to update.

Copy link
Copy Markdown
Member Author

@juliemr juliemr Feb 17, 2016

Choose a reason for hiding this comment

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

I don't think perf should be an issue here, it's certainly not a hot code path.

Copy link
Copy Markdown

@kegluneq kegluneq Feb 17, 2016

Choose a reason for hiding this comment

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

In that case, I'd vote removing the check altogether and collapsing these 4 lines into 1.

Copy link
Copy Markdown
Member Author

@juliemr juliemr Feb 17, 2016

Choose a reason for hiding this comment

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

Actually, we do need the check here - we don't want to call addProviders at all if there's no new ones, to avoid throwing an error if we've already instantiated the injector.

@kegluneq kegluneq added pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 17, 2016
@kegluneq
Copy link
Copy Markdown

kegluneq commented Feb 17, 2016

LGTM

@juliemr
Copy link
Copy Markdown
Member Author

juliemr commented Feb 17, 2016

Thanks!

@juliemr juliemr added the action: merge The PR is ready for merge by the caretaker label Feb 17, 2016
@juliemr juliemr assigned matsko and unassigned btford Feb 17, 2016
@matsko
Copy link
Copy Markdown
Contributor

matsko commented Feb 20, 2016

Landed as c1a0af5

@matsko matsko closed this Feb 20, 2016
@angular-automatic-lock-bot
Copy link
Copy Markdown

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

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

Reviewers

No reviews

Assignees

@matsko matsko

Labels

action: merge The PR is ready for merge by the caretaker area: testing Issues related to Angular testing features, such as TestBed cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat(test): ability to configure providers on a per-spec basis

7 participants

@juliemr @IgorMinar @jelbourn @kegluneq @matsko @btford @googlebot