Skip to content

fix(ivy): apply all overrides from TestBed, not the last one only - #27734

Closed
AndrewKushnir wants to merge 1 commit into
angular:masterfrom
AndrewKushnir:FW-852_testbed_override_pipe
Closed

AndrewKushnir wants to merge 1 commit into
angular:masterfrom
AndrewKushnir:FW-852_testbed_override_pipe

Conversation

@AndrewKushnir

Copy link
Copy Markdown
Contributor

In some cases in our tests we can define multiple overrides for a given class. As a result, only the last override is actually applied due to the fact that we store overrides in a Type<->Override map. This update changes the logic to keep all overrides defined in a given test for a Type (i.e. Type<->Override[] map) and applies them one by one at resolution phase. This behavior is more inline with the previous TestBed.

An example test case (from packages/platform-browser/test/testing_public_spec.ts) that was failing is:

    TestBed
        .overridePipe(SomePipe, {set: {name: 'somePipe'}})
        .overridePipe(SomePipe, {add: {pure: false}});

As a result, the pure flag was changed, but the name remained the same, because the name-related override was replaced by the second one that sets the pure flag.

This PR resolves FW-852.

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

In some cases in our tests we can define multiple overrides for a given class. As a result, only the last override is actually applied due to the fact that we store overrides in a Type<->Override map. This update changes the logic to keep all overrides defined in a given test for a Type (i.e. Type<->Override[] map) and applies them one by one at resolution phase. This behavior is more inline with the previous TestBed.
@AndrewKushnir AndrewKushnir added type: bug/fix 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 target: major This PR is targeted for the next major release comp: ivy labels Dec 18, 2018
@ngbot ngbot Bot added this to the needsTriage milestone Dec 18, 2018
@mary-poppins

Copy link
Copy Markdown

You can preview 9a8b4fc at https://pr27734-9a8b4fc.ngbuilds.io/.

@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

compFixture.detectChanges();
expect(compFixture.nativeElement).toHaveText('transformed hello');
});
it('should work', () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This test name makes me sad :(

@AndrewKushnir

Copy link
Copy Markdown
Contributor Author

Presubmit

@AndrewKushnir
AndrewKushnir removed the request for review from pkozlowski-opensource December 20, 2018 19:12
@AndrewKushnir AndrewKushnir 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 20, 2018
@matsko matsko closed this in 509aa61 Dec 21, 2018
matsko added a commit to matsko/angular that referenced this pull request Dec 26, 2018
@angular-automatic-lock-bot

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.

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 target: major This PR is targeted for the next major release type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants