Skip to content

Copy-paste gaps in rule tests: DateTime/Time tests check Date, and Integer has no contract tests #826

Description

@roxblnfk

What steps will reproduce the problem?

1. DateTimeTest and TimeTest test the Date rule instead of their own rule.

Three methods in tests/Rule/Date/DateTimeTest.php and tests/Rule/Date/TimeTest.php create new Date() instead of new DateTime() / new Time():

public function testGetName(): void
{
    $rule = new Date();                      // should be new DateTime() / new Time()
    $this->assertSame('date', $rule->getName());
}

public function testSkipOnError(): void
{
    $this->testSkipOnErrorInternal(new Date(), new Date(skipOnError: true));
}

public function testWhen(): void
{
    $this->testWhenInternal(
        new Date(),
        new Date(when: static fn(mixed $value): bool => $value !== null),
    );
}

So getName(), skipOnError and when are never tested for DateTime and Time. DateTest already covers Date, so these checks only run twice more on the same rule.

2. The Integer rule has no tests for getName(), skipOnError and when.

Integer has no test class of its own. NumberTest checks how Integer validates values (dataValidationPassed()/dataValidationFailed()), but testGetName(), testSkipOnError() and testWhen() there only use new Number(). AbstractNumber::getName() returns static::class, so Integer returns its own name, and no test checks that.

What is the expected result?

Every rule has these contract checks run on an instance of that rule.

What do you get instead?

DateTime, Time and Integer are not covered. A regression in their getName(), skipOnError() or when() would go unnoticed. For example: an override in the class, a missing trait after a refactoring, or a wrong default in the constructor.

Suggested fix

  • In DateTimeTest/TimeTest, replace Date with DateTime/Time in the three methods above. getName() returns 'date' for all of them (from BaseDate), so the expected value stays the same.

  • Add testIntegerGetName(), testIntegerSkipOnError() and testIntegerWhen() to NumberTest, or split them into a separate IntegerTest:

    public function testIntegerSkipOnError(): void
    {
        $this->testSkipOnErrorInternal(new Integer(), new Integer(skipOnError: true));
    }
  • Optionally, prevent this kind of copy-paste mistake: add one data-provider test that runs the skipOnError/when checks over a list of all rules, instead of a separate method in each test class. Then adding a new rule means adding one line to the provider. The #[DataProvider] returns rule × option pairs (skipOnError, when); each pair gives a factory, the option name, its default, a new value and the getter name.

Additional info

Q A
Version 2.x-dev (9be0c31)
PHP version any
Operating system any

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions