Skip to content

[Validator] Add CompoundConstraintTestCase to ease testing Compound Constraints #49547

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

Merged

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Feb 27, 2023

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #50205
License MIT
Doc PR Todo

This PR aims to ease compound constraints to be tested.

Real world use case: we're using the Validator component to test password strength and if they match our password policy. We created a compound constraint for this. This PR allows to write easy and understandable specific tests of compound constraints, like this:

// src/Validator/MatchPasswordPolicy.php
use Symfony\Component\Validator\Constraints as Assert;

#[\Attribute]
class MatchPasswordPolicy extends Assert\Compound
{
    protected function getConstraints(array $options): array
    {
        return [
            new Assert\NotBlank(allowNull: false),
            new Assert\Length(min: 8, max: 255),
            new Assert\NotCompromisedPassword(),
            new Assert\Type('string'),
            new Assert\Regex('/[A-Z]+/'),
            // and a few other but you get it
        ];
    }
}

Testing this constraint with this new TestCase helper class would result on a clear test classe:

// tests/Validator/MatchPasswordPolicyTest.php
use Symfony\Component\Validator\Constraints as Assert;
use Symfony\Component\Validator\Test\CompoundConstraintTestCase;

class MatchPasswordPolicyTest extends CompoundConstraintTestCase
{
    public function createCompound(): Assert\Compound
    {
        return new MatchPasswordPolicy();
    }

    /**
     * @dataProvider provideInvalidPasswords
     */
    public function testInvalid(mixed $password, string $code): void
    {
        $this->validateValue($password);

        $this->assertViolationIsRaisedByCompound($code);
    }

    public static function provideInvalidPasswords(): \Generator
    {
        yield 'Blank' => ['', Assert\NotBlank::IS_BLANK_ERROR];

        yield 'Too short' => ['a', Assert\Length::TOO_SHORT_ERROR];

        yield 'Too long' => [/** Generate long string */, Assert\Length::TOO_LONG_ERROR];

        yield 'Not a string' => [1, Assert\Type::INVALID_TYPE_ERROR];

        yield 'No lowercase' => ['UPPER_CHAR#1', Assert\Regex::REGEX_FAILED_ERROR];

        yield 'No uppercase' => ['lower_char#1', Assert\Regex::REGEX_FAILED_ERROR];

        yield 'No digit' => ['no_digit_pass#', Assert\Regex::REGEX_FAILED_ERROR];
    }

    public function testValid(): void
    {
        $this->validateValue('VeryStr0ngP4$$wOrD');

        $this->assertNoViolation();
    }
}

@carsonbot carsonbot added this to the 6.3 milestone Feb 27, 2023
@alexandre-daubois alexandre-daubois force-pushed the compound-constraint-test-case branch 2 times, most recently from 2aeaf5b to cd080f1 Compare February 28, 2023 12:50
@alexandre-daubois alexandre-daubois force-pushed the compound-constraint-test-case branch 2 times, most recently from 87365e3 to dac2687 Compare February 28, 2023 19:48
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@sylfabre
Copy link
Contributor

Hello @alexandre-daubois
Thank you for tackling this issue 👍
Are you still working on this PR?

If not (I totally understand), @stof @nicolas-grekas @xabbuh I suggest making ConstraintValidatorTestCase throw an exception when used with the Compound constraint with a link to this PR. Right now it is very misleading:

  • A test may actually pass without any check
  • A test may be false without reason

I lost about two hours yesterday because of that (not blaming anyone) 😬

@alexandre-daubois
Copy link
Member Author

Hi, thanks for the interest in this PR! I'd still love to see this happen in the framework. I'll rebase it on 6.4 🙂

@alexandre-daubois alexandre-daubois force-pushed the compound-constraint-test-case branch 2 times, most recently from 1ddaebc to 3f7261c Compare August 7, 2023 14:17
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@alexandre-daubois alexandre-daubois force-pushed the compound-constraint-test-case branch 2 times, most recently from 878105b to cf20e03 Compare August 2, 2024 11:26
@alexandre-daubois
Copy link
Member Author

I just rebased in 7.2, if anyone wants to have a look 🙂

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM with minor CS changes. Please rebase also.

@alexandre-daubois alexandre-daubois force-pushed the compound-constraint-test-case branch from cf20e03 to 7d18268 Compare August 22, 2024 09:37
@alexandre-daubois alexandre-daubois changed the title [Validator] Add CompoundConstraintTestCase to ease testing Compound Constraint [Validator] Add CompoundConstraintTestCase to ease testing Compound Constraints Aug 22, 2024
@alexandre-daubois alexandre-daubois force-pushed the compound-constraint-test-case branch from 7d18268 to 73de44d Compare August 22, 2024 09:38
@alexandre-daubois alexandre-daubois force-pushed the compound-constraint-test-case branch 2 times, most recently from 532f838 to d196c22 Compare August 22, 2024 09:40
Comment on lines 75 to 80
$validator->validate($this->validatedValue, new class($constraints) extends Compound {
public function __construct(array $constraints)
{
parent::__construct(['constraints' => $constraints]);
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$validator->validate($this->validatedValue, new class($constraints) extends Compound {
public function __construct(array $constraints)
{
parent::__construct(['constraints' => $constraints]);
}
$validator->validate($this->validatedValue, new class(['constraints' => $constraints]) extends Compound {

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this solution (and the other one also unfortunately) doesn't work, it raises an exception:

Symfony\Component\Validator\Exception\ConstraintDefinitionException : You can't redefine the "constraints" option. Use the "Symfony\Component\Validator\Constraints\Compound::getConstraints()" method instead.

Copy link
Member

Choose a reason for hiding this comment

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

ah!

@alexandre-daubois alexandre-daubois force-pushed the compound-constraint-test-case branch from d196c22 to 682d866 Compare August 22, 2024 09:44
$validator->initialize($context);

$validator->validate($this->validatedValue, new class($constraints) extends Compound {
public function __construct(public array $constraints)
Copy link
Member

Choose a reason for hiding this comment

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

You redeclare and assign the $constraints property to use it in the getConstraints() implementation which is called by the parent class to populate the $constraints property. This is very confusing.

I suggest we find a different name for a local $constraints property and make that property private.

Copy link
Member Author

@alexandre-daubois alexandre-daubois Aug 22, 2024

Choose a reason for hiding this comment

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

Got it. What do you think of this new naming I just pushed, $testedConstraints?

Copy link
Member

Choose a reason for hiding this comment

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

👌🏻

@alexandre-daubois alexandre-daubois force-pushed the compound-constraint-test-case branch from 682d866 to 14f3252 Compare August 22, 2024 09:59
@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit b7fe7ef into symfony:7.2 Aug 22, 2024
9 of 10 checks passed
fabpot added a commit that referenced this pull request Aug 22, 2024
…ructor (derrabus)

This PR was merged into the 7.2 branch.

Discussion
----------

[Validator] Add $groups and $payload to Compound constructor

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | N/A
| License       | MIT

While reviewing #49547 I noticed that the `$groups` and `$payload` parameters introduced in #41994 cannot be leveraged in compound constraints. I'd like to change that.

Commits
-------

bf4207c [Validator] Add $groups and $payload to Compound constructor
@fabpot fabpot mentioned this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Validator] Testing compound constraints
8 participants