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

fix(core): use addCustomEqualityTester instead of overriding toEqual #22983

Closed
wants to merge 3 commits into from

Conversation

KevinVillela
Copy link
Contributor

This propagates other custom equality testers added by users. Additionally, if
an Angular project is using jasmine 2.6+, it will allow Jasmine's custom object
differ to print out pretty test error messages.

fixes #22939

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No
[x] Kind of?

It may break (well, fix) tests that add matchers which were not being invoked. Now that those matchers will be invoked, some tests that weren't testing things correctly will start to do so, and that could cause test failures.

Other information

@IgorMinar IgorMinar added the area: testing Issues related to Angular testing features, such as TestBed label Mar 29, 2018
@KevinVillela
Copy link
Contributor Author

Friendly ping, I know y'all are busy but this PR seems pretty innocuous, and I think a lot of people would want to have their nice jasmine differs and custom equality checkers back.

@mhevery mhevery added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker labels Jul 2, 2018
@mhevery
Copy link
Contributor

mhevery commented Jul 2, 2018

@KevinVillela
Copy link
Contributor Author

Ah dang, so all these tests pass but the google3 ones don't? Is there action required by me to make that work, or is that something the project maintainers handle?

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Jul 2, 2018
@mhevery mhevery added cla: yes and removed cla: no labels Jul 2, 2018
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@mhevery
Copy link
Contributor

mhevery commented Jul 2, 2018

I think 5671dcc will fix it...

@mhevery
Copy link
Contributor

mhevery commented Jul 2, 2018

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Jul 2, 2018
@mhevery mhevery added cla: yes and removed cla: no labels Jul 2, 2018
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@mhevery
Copy link
Contributor

mhevery commented Jul 2, 2018

KevinVillela and others added 3 commits July 2, 2018 15:43
This propagates other custom equality testers added by users. Additionally, if
an Angular project is using jasmine 2.6+, it will allow Jasmine's custom object
differ to print out pretty test error messages.

fixes angular#22939
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot removed the cla: yes label Jul 2, 2018
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

mhevery pushed a commit that referenced this pull request Jul 3, 2018
…22983)

This propagates other custom equality testers added by users. Additionally, if
an Angular project is using jasmine 2.6+, it will allow Jasmine's custom object
differ to print out pretty test error messages.

fixes #22939

PR Close #22983
@mhevery mhevery closed this in 0922228 Jul 3, 2018
@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 Sep 13, 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: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom toEqual matcher doesn't propagate custom equality testers
4 participants