Skip to content

Commit

Permalink
fix nondeterminism in OptionalMethodCallProcessor
Browse files Browse the repository at this point in the history
An optional method call (`__calls : [ setLocation (80%?): [40.689269,
-74.044737] ]`) would be resolved using an external source of
randomness, rather than the FakerGenerator instance used by all other
value generators, resulting in nondeterministic behavior.

We're making the generator an optional parameter to
OptionalMethodCallProcessor and falling back to random_int for
backwards-compatibility. This can be made non nullable in a next major
release.

(Also see OptionalValueResolver, this is handled in the same way there
https://github.com/nelmio/alice/blob/a7e15b08a64d4f6fb64b4c3b58bd6a710f71050e/src/Generator/Resolver/Value/Chainable/OptionalValueResolver.php#L101-L103)

Fixes gh-1230
  • Loading branch information
brainlock committed Dec 20, 2024
1 parent 5b87f64 commit c55be85
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 4 deletions.
19 changes: 16 additions & 3 deletions src/Generator/Caller/Chainable/OptionalMethodCallProcessor.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

namespace Nelmio\Alice\Generator\Caller\Chainable;

use Faker\Generator as FakerGenerator;
use LogicException;
use Nelmio\Alice\Definition\MethodCall\OptionalMethodCall;
use Nelmio\Alice\Definition\MethodCallInterface;
Expand All @@ -33,14 +34,20 @@ final class OptionalMethodCallProcessor implements ChainableCallProcessorInterfa
*/
private $processor;

public function __construct(?CallProcessorInterface $processor = null)
/**
* @var ?FakerGenerator
*/
private $faker;

public function __construct(?CallProcessorInterface $processor = null, ?FakerGenerator $faker = null)
{
$this->processor = $processor;
$this->faker = $faker;
}

public function withProcessor(CallProcessorInterface $processor): self
{
return new self($processor);
return new self($processor, $this->faker);
}

public function canProcess(MethodCallInterface $methodCall): bool
Expand All @@ -65,7 +72,13 @@ public function process(
throw new LogicException('TODO');
}

if (random_int(0, 99) >= $methodCall->getPercentage()) {
// TODO: for BC purposes, let's fallback to random_int. The generator should be made
// non-nullable in 4.x and the random_int usage removed. (See OptionalValueResolver)
$random = null !== $this->faker
? $this->faker->numberBetween(0, 99)
: random_int(0, 99);

if ($random >= $methodCall->getPercentage()) {
return $fixtureSet;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Loader/NativeLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,7 @@ protected function createCallProcessor(): CallProcessorInterface
return new CallProcessorRegistry([
new ConfiguratorMethodCallProcessor(),
new MethodCallWithReferenceProcessor(),
new OptionalMethodCallProcessor(),
new OptionalMethodCallProcessor(null, $this->getFakerGenerator()),
new SimpleMethodCallProcessor(),
]);
}
Expand Down
28 changes: 28 additions & 0 deletions tests/Loader/LoaderIntegrationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
use Nelmio\Alice\UserDetail;
use PHPUnit\Framework\TestCase;
use ReflectionClass;
use ReflectionObject;
use stdClass;
use Throwable;
use TypeError;
Expand Down Expand Up @@ -1575,6 +1576,33 @@ public function testNewlinesInIdentity(): void
);
}

public function testOptionalMethodCallsAreDeterministic(): void
{
$gen_sequence = function (NativeLoader $loader) {
$set = $loader->loadData([
FixtureEntity\Caller\Dummy::class => [
'dummy{0..9}' => [
'__construct' => false,
'__calls' => [
['setTitle (50%?)' => ['Foo']],
],
],
],
]);
$objs = array_values($set->getObjects());

return array_map(
fn ($obj) => (new ReflectionObject($obj))->getProperty('title')->getValue($obj),
$objs,
);
};

$seq_1 = $gen_sequence(new NativeLoader());
$seq_2 = $gen_sequence(new NativeLoader());

self::assertSame($seq_1, $seq_2);
}

public static function provideFixturesToInstantiate(): iterable
{
yield 'with default constructor – use default constructor' => [
Expand Down

0 comments on commit c55be85

Please sign in to comment.