Skip to content

Be strict about data providers having redundant data#6213

Merged
sebastianbergmann merged 2 commits intosebastianbergmann:mainfrom
6b7562617765726c6f73:be_strict_about_data_providers_having_redundant_data
May 20, 2025
Merged

Be strict about data providers having redundant data#6213
sebastianbergmann merged 2 commits intosebastianbergmann:mainfrom
6b7562617765726c6f73:be_strict_about_data_providers_having_redundant_data

Conversation

@kubawerlos
Copy link
Copy Markdown
Contributor

@kubawerlos kubawerlos commented May 20, 2025

Closes #6211

  • 1st commit is the feature itself
  • 2nd commit is the changes to align the codebase to follow the new rule

Should this be hidden behind a config option?
Should it make the test risky/with a warning rather than producing error?

@sebastianbergmann
Copy link
Copy Markdown
Owner

Should this be hidden behind a config option?

No.

@sebastianbergmann sebastianbergmann added the type/enhancement A new idea that should be implemented label May 20, 2025
@sebastianbergmann sebastianbergmann added this to the PHPUnit 12.2 milestone May 20, 2025
@sebastianbergmann sebastianbergmann merged commit 6d65e2e into sebastianbergmann:main May 20, 2025
25 checks passed
@sebastianbergmann
Copy link
Copy Markdown
Owner

Thank you!

@kubawerlos kubawerlos deleted the be_strict_about_data_providers_having_redundant_data branch May 20, 2025 12:56
@kubawerlos
Copy link
Copy Markdown
Contributor Author

@sebastianbergmann you are so fast 👍🏼

Even too fast: #6215

@sebastianbergmann
Copy link
Copy Markdown
Owner

I have "downgraded" this from an error to a warning in 7f1969a.

@kubawerlos
Copy link
Copy Markdown
Contributor Author

Cool. The warning will be on the test case level, not at the test method level, as the error was.

@sebastianbergmann
Copy link
Copy Markdown
Owner

@dereuromark
Copy link
Copy Markdown

dereuromark commented Jun 6, 2025

This might have a small regression for ...

    /**
     * tests failure messages for assertions
     *
     * @param string $assertion Assertion method
     * @param string $message Expected failure message
     * @param string $url URL to test
     * @param mixed ...$rest
     */
    #[DataProvider('assertionFailureMessagesProvider')]
    public function testAssertionFailureMessages($assertion, $message, $url, ...$rest): void
    {
        $this->expectException(AssertionFailedError::class);
        $this->expectExceptionMessage($message);

        Security::setSalt($this->key);

        $this->get($url);

        call_user_func_array($this->$assertion(...), $rest);
    }

If there is a ...$var maybe the warning should be skipped as it is intentional?

1) Cake\Test\TestCase\TestSuite\IntegrationTestTraitTest::testAssertionFailureMessages
* Data set "assertCookie" has more arguments (5) than the test method accepts (4)

@dereuromark
Copy link
Copy Markdown

Maybe a

$parameters = $method->getParameters();

$hasVariadic = false;

foreach ($parameters as $param) {
    if ($param->isVariadic()) {
        $hasVariadic = true;
        break;
    }
}

And if true we just skip?

@sebastianbergmann
Copy link
Copy Markdown
Owner

@dereuromark Please open a new issue if you think you found a bug/regression. Thank you.

@kubawerlos
Copy link
Copy Markdown
Contributor Author

It is regression, I've raised the PR with the fix: #6228

theofidry added a commit to theofidry/console-parallelization that referenced this pull request Aug 19, 2025
theofidry added a commit to webmozarts/console-parallelization that referenced this pull request Aug 19, 2025
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 14, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

feature/data-provider Data Providers type/enhancement A new idea that should be implemented

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warn about impossible data provider

3 participants