pestphp / pestphp/pest

[Bug]: ->with([A::class, B::class]) is called as a [class, method] callable instead of iterated, dropping the test

Open Beginner friendly
#1,902 1 comment 2 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
PHP
Stars
11.7k
Forks
538
Avg merge
4d 11h
Merged PRs (30d)
8

Description

What Happened

A dataset written as an unkeyed pair of class-name strings is invoked as a callable instead of iterated as two rows. PHP reads a two-element array whose first element names a class as a [class, method] callable, and is_callable() accepts any method name on a class that defines __callStatic. DatasetsRepository::processDatasets() uses is_callable(), so it calls the dataset:

https://github.com/pestphp/pest/blob/main/src/Repositories/DatasetsRepository.php

if (is_string($data)) {
    $datasets[$index] = self::getScopedDataset($data, $currentTestFile);
}

if (is_callable($datasets[$index])) {          // <-- true for [SomeClass::class, 'anything']
    $datasets[$index] = call_user_func($datasets[$index]);
}

Whatever __callStatic does then decides the outcome. The data provider ends up invalid and every test behind the dataset is dropped from the run.

Expected: two tests, ('DatasetRepoAlpha') and ('DatasetRepoBeta').

The shape is narrow, which is what makes it easy to write and easy to miss in review:

  • exactly two elements — three or more is not a callable shape and works correctly;
  • unkeyed['a' => A::class, 'b' => B::class] works correctly;
  • first element naming a real class that can answer the second (__callStatic means any name qualifies).

This bites hardest in Laravel, where every Eloquent model has __callStatic. ->with([UserA::class, UserB::class]) forwards to a query builder, and because data providers run at suite-build time before the framework has booted, it dies with Call to a member function connection() on null — an error that points nowhere near the dataset.

How to Reproduce

Self-contained, no framework needed. DatasetRepoAlpha::__callStatic should never run, and does:

<?php

class DatasetRepoAlpha
{
    public static function __callStatic(string $name, array $arguments): never
    {
        throw new RuntimeException("__callStatic({$name}) should never have been reached");
    }
}

class DatasetRepoBeta {}

test('a plain test, so the run is not empty', function () {
    expect(true)->toBeTrue();
});

test('two rows', function (string $class) {
    expect(class_exists($class))->toBeTrue();
})->with([DatasetRepoAlpha::class, DatasetRepoBeta::class]);
   FAIL  Tests\Feature\TmpUpstreamReproTest
  ✓ a plain test, so the run is not empty                                5.60s
  ────────────────────────────────────────────────────────────────────────────
   FAILED  Tests\Feature\TmpUpstreamReproTest >
  The data provider P\Tests\Feature\TmpUpstreamReproTest::__pest_evaluable_two_rows_dataset specified for P\Tests\Feature\TmpUpstreamReproTest::__pest_evaluable_two_rows is invalid
__callStatic(DatasetRepoBeta) should never have been reached

  Tests:    1 failed, 1 passed (1 assertions)

Removing the plain test, so the dataset test is the only one in the run, reports this instead — exit code is still 2, but nothing says why:

   INFO  No tests found.

Any of these three variants of the same dataset behaves correctly, which is how the bug hides:

->with([A::class, B::class, C::class]);           // three rows: not a callable shape
->with(['a' => A::class, 'b' => B::class]);       // keyed
->with([[A::class], [B::class]]);                 // rows wrapped
Sample Repository

Not needed — the snippet above is self-contained.

Pest Version

4.7.2 (PHPUnit 12.5.28)

PHP Version

8.5.10

Operation System

Linux

Notes

Pest\PendingCalls\TestCall::with() is typed Closure|iterable|string ...$data, so the only callable it can legitimately be handed is a Closure. Narrowing the check to match the signature rejects the array shape without affecting closure or named-dataset datasets:

-        if (is_callable($datasets[$index])) {
-            $datasets[$index] = call_user_func($datasets[$index]);
+        if ($datasets[$index] instanceof Closure) {
+            $datasets[$index] = ($datasets[$index])();
         }

is_string() on the preceding line already covers named datasets (and getScopedDataset() can return a Closure, which the narrowed check still calls), and Traversable is handled just below — so nothing else there depends on is_callable(). The one behaviour that would change is passing an invokable object as a dataset; I could not find that documented, but worth confirming.

Happy to send a PR for this if the approach looks right.

Related: #767 covered the general "dataset errors are quietly suppressed" symptom, and that is genuinely fixed — the invalid provider is reported now, as above. The residue is the empty-run case, where No tests found replaces the explanation.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/Repositories/DatasetsRepository.php at DatasetsRepository::processDatasets(), then run the self-contained reproduction from the issue. Confirm that the two unkeyed class names produce two dataset rows without invoking DatasetRepoAlpha::__callStatic, while closures and named datasets still work.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
testing-qa
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
84/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.