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

Added Symfony validator assertions #189

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

TavoNiievez
Copy link
Member

@TavoNiievez TavoNiievez commented Mar 26, 2024

Examples:

        $user = User::create('test@example.com', 'password', ['ROLE_ADMIN']);

        $I->dontSeeViolatedConstraint($user);
        $I->dontSeeViolatedConstraint($user, 'email');
        $I->dontSeeViolatedConstraint($user, 'email', Email::class);
        $I->dontSeeViolatedConstraint($user, 'password', NotBlank::class);

        $user->setPassword('weak');
        $I->seeViolatedConstraint($user, 'password', Length::class);
        $user = User::create('invalid_email', 'weak', ['ROLE_ADMIN']);

        $I->seeViolatedConstraintsCount(2, $user);
        $I->seeViolatedConstraintsCount(1, $user, 'email');
        $I->seeViolatedConstraintMessage('is not a valid email', $user, 'email');

        $user->setEmail('');
        $I->seeViolatedConstraintMessage('should not be blank', $user, 'email');

@TavoNiievez
Copy link
Member Author

@cs278 @W0rma @xEdelweiss @ThomasLandauer

These assertions verify that the Symfony validator component is working correctly in the application, specifically with the test data defined in the test cases.

If you have some free time please review my PR :-)
Thanks!

@ThomasLandauer
Copy link
Member

What about shortening the name a bit, from dontSeeViolatedConstraint to just dontSeeViolation?
And I think the "count" method should be plural: seeViolationsCount

@TavoNiievez
Copy link
Member Author

TavoNiievez commented Mar 26, 2024

Hi @ThomasLandauer
Indeed, I considered seeViolation and seeViolated but I think that when reading it is not explicit with what 'violation' I am referring to, so I wanted to avoid uncomfortable laughs or unintentional offenses in the developers who read this code.

For the moment, I agree that the "count" method should be plural, thank you very much for pointing that out.

@ThomasLandauer
Copy link
Member

IMO, "violation" is a common term in programming, so I guess it's politically correct :-) dontSeeViolatedConstraint is OK too, it's just that the method names are getting longer and longer...
What do the others say?

@burned42
Copy link
Contributor

If I think in terms of Symfony, validator constraints is a pretty commonly known naming, so I think my first guess reading dontSeeViolation would also be that it's about violation constraints from the validator component.
But that naming might not be that clear for everybody and I think using the naming violation constraint might be more explicit and if in doubt I tend to rather use longer naming than being ambiguous.
In the end, I don't have a strong preference here, because in Codeception I intentionally have to enable the Symfony module, so I'm aware of the Symfony context.

FYI, Symfony itself also doesn't use 'constraint' in the method naming, but then again, in the Symfony context that might not be necessary: https://github.com/symfony/symfony/blob/7.0/src/Symfony/Component/Validator/Test/ConstraintValidatorTestCase.php

@xEdelweiss
Copy link
Contributor

Hi @TavoNiievez
I am a little late, yet this is a great addition to the module!

I also like the shorter names, and the Symfony Serializer uses 'violations' as a key in normalization, so it should be easy to understand.

On the other hand, a longer name makes it easier to discover the method without looking at the docs. At least I try to "guess" the appropriate method first :)
It's nice that you may find it by searching for either 'constraint' or 'violation'.

@TavoNiievez TavoNiievez merged commit 36e08c9 into Codeception:main Apr 5, 2024
8 checks passed
@TavoNiievez TavoNiievez added this to the 3.4.0 milestone Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants