Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Validator][Tests] Fix AssertingContextualValidator not throwing on remaining expectations#42458

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@fancyweb
Copy link
Contributor

@fancywebfancyweb commentedAug 9, 2021•
edited
Loading

QA
Branch?4.4
Bug fix?no
New feature?no
Deprecations?no
Tickets-
LicenseMIT
Doc PR-

While working on something I noticed two missing things in theAssertingContextualValidator.

  1. We don't check if there are remaining expected calls when theAssertingContextualValidator is destroyed. Therefore the tests using it always pass, whether the validator actually callsvalidate or not. (for example: comment the->validate() line inAllValidator and tests still pass)
    2. When the expected value / value is an object, we should useassertEquals because it cannot logically be the same instance.

Ping@xabbuh

@carsonbotcarsonbot added this to the4.4 milestoneAug 9, 2021
@carsonbotcarsonbot changed the title[Validator][Tests] Improve AssertingContextualValidator checks[Validator] [Tests] Improve AssertingContextualValidator checksAug 9, 2021
@carsonbot
Copy link

Hey!

I think@przemyslaw-bogusz has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

LGTM but I'm not sure about theAssertEquals() part for objects. /cc@xabbuh

@fancywebfancywebforce-pushed thevalidator/test-assert-contextual branch from9b458d2 to27d0fdaCompareNovember 25, 2021 13:43
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 28, 2021•
edited
Loading

Shouldn't this be covered by some tests?
/cc@xabbuh available for a review please? 🙏

@fancywebfancywebforce-pushed thevalidator/test-assert-contextual branch from27d0fda tob409cb2CompareDecember 31, 2021 10:31
@fancywebfancywebforce-pushed thevalidator/test-assert-contextual branch fromb409cb2 toaac1013CompareDecember 31, 2021 10:32
@fancywebfancyweb changed the title[Validator] [Tests] Improve AssertingContextualValidator checks[Validator][Tests] Fix AssertingContextualValidator not throwing on remaining expectationsDec 31, 2021
@fancyweb
Copy link
ContributorAuthor

I added a test. I reverted the assertEquals() for objects because it's in the case where you mutate the validated value in the validator (for example, the value in the validator is a string, you construct a \DateTime from it and you validate the \DateTime instance). I guess that could be considered as a bad practice so I decided I don't care for now.

@fancyweb
Copy link
ContributorAuthor

@fabpot@xabbuh@nicolas-grekas I'm attempting another ping on this one because it's a real bug and I would like it to be fixed 😅

@nicolas-grekas
Copy link
Member

Thank you@fancyweb.

@nicolas-grekasnicolas-grekas merged commit75bf7fa intosymfony:4.4Feb 23, 2022
@fancywebfancyweb deleted the validator/test-assert-contextual branchFebruary 23, 2022 13:25
This was referencedFeb 28, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

4 participants

@fancyweb@carsonbot@nicolas-grekas@fabpot
[8]ページ先頭
©2009-2025 Movatter.jp