Skip to content

Improve Test Trait Method Names - #45

Open
Neluxx wants to merge 8 commits into
cakephp-5.xfrom
44-improve-method-names
Open

Neluxx wants to merge 8 commits into
cakephp-5.xfrom
44-improve-method-names

Conversation

@Neluxx

@Neluxx Neluxx commented Sep 22, 2026

Copy link
Copy Markdown
Member

Closes #44

@Neluxx
Neluxx requested a review from ravage84 September 22, 2026 06:20
@Neluxx Neluxx self-assigned this Sep 22, 2026
@ravage84 ravage84 added this to the 3.0 milestone Sep 22, 2026
@ravage84

Copy link
Copy Markdown
Member

As discussed, the documentation adjustments are not done, yet.

@ravage84

Copy link
Copy Markdown
Member

Also as discussed, we should update the target branch once we agreed on a naming.

@ravage84 ravage84 left a comment

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.

🍟

static::assertSame($expectedErrors, $duplicate->getError($field));

$this->testDataRules($this->table, $field, $dataSet, $expectedErrors);
$this->assertDataRules($this->table, $field, $dataSet, $expectedErrors);

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.

Let's remove the Data from DataRules, too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's not possible because we had a testDataRules and testRules methode before.

These methods became assertDataRules and assertRules.

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.

OK, thanks.

But I'm confused now as why we have these two methods in the first place because they almost look and read the same.

assertDataRules(	Table $table, string $fieldName, array $dataSet, array $expected,	array $options = []
assertRules(		Table $table, string $fieldName, array $dataSet, array $expected,	array $options = []
assertRulesNoErrors(Table $table, string $fieldName, array $dataSet,					array $options = []

If you compare both methods in their initial state:

    /**
     * Validate that a given data set for a given table leads to the expected rule errors
     *
     * @param Table $table The table to test
     * @param string $fieldName The field to check for errors.
     * @param array $dataSet The data set to test.
     * @param array $expected The expected errors.
     * @param array $options Additional options for newEntity.
     * @return void
     * @todo Move to a rules dedicated helper class.
     */
    protected function _testDataRules(
        Table $table,
        string $fieldName,
        array $dataSet,
        array $expected,
        array $options = []
    ) {
        $defaultOptions = ['validate' => false];
        $options = $defaultOptions + $options;
        $entity = $table->newEntity($dataSet, $options);

        $entitySaved = $table->save($entity, $options);
        static::assertFalse($entitySaved);

        $errors = $entity->getError($fieldName);
        static::assertEquals($expected, $errors);
    }

vs.

    /**
     * Validate that a given data set for a given table leads to the expected table rules errors
     *
     * @param Table $table The table to test
     * @param string $fieldName The field to check for table rules errors.
     * @param array $dataSet The data set to test.
     * @param array $expected The expected table rules errors.
     * @param array $options Additional options for newEntity.
     * @return void
     */
    protected function _testRules(
        Table $table,
        string $fieldName,
        array $dataSet,
        array $expected,
        array $options = [])
    {
        $entity = $table->newEntity($dataSet, $options);
        $errors = $entity->getError($fieldName);
        static::assertEmpty($errors);

        $table->save($entity);
        $errors = $entity->getError($fieldName);
        static::assertEquals($expected, $errors);
    }

I suspect they are a copy-pasta error. Each method originated in a slightly different form from a different application project and both ended up side-by-side in our open source project here.

I think we only need assertDataRules() but named assertRules(), as it merges the options with the disabled validation and thus is similar to assertRulesNoErrors().

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The difference is that assertRules() first validates the entity and only checks the rules if the entity is valid. The assertDataRules() method disables entity validation and checks only the rules.

Good question if we really need to make that distinction? If so, I would rename the method names to assertRulesWithValidation() and assertRulesOnly() so their purpose is clear.

However, we could also rename assertDataRules() to assertRules() and remove the old assertRules() method.

Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md Outdated
*/
protected function testFullDataValidation(Table $table, array $dataSet, array $expected, array $options = []): void
{
protected function assertFullDataValidation(

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.

Should we rename this and the other "full" one, too?

I'm not sure Full is the best description.

- `testFullDataValidation($table, $dataSet, $expected)` - asserts errors across all fields.
- `testFullDataValidationNoErrors($table, $dataSet)` - asserts a full data set produces no errors at all.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree with you there. Do you have any ideas? How about assertValidationErrors() and assertNoValidationErrors()?

Comment thread CHANGELOG.md Outdated
@Neluxx
Neluxx requested a review from ravage84 September 23, 2026 06:47
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.

Improve Test Trait Method Names

2 participants