[player] make --blackfire-env optionnal

Author: julien-jean

Player/Console/PlayerCommand.php

@@ -103,7 +103,7 @@ protected function configure(): void
                 new InputOption('disable-internal-network', '', InputOption::VALUE_NONE, 'Disable internal network'),
                 new InputOption('sandbox', '', InputOption::VALUE_NONE, 'Enable the sandbox mode'),
                 new InputOption('ssl-no-verify', '', InputOption::VALUE_NONE, 'Disable SSL certificate verification'),
-                new InputOption('blackfire-env', '', InputOption::VALUE_REQUIRED, 'The blackfire environment to use'),
+                new InputOption('blackfire-env', '', InputOption::VALUE_OPTIONAL, 'The blackfire environment to use'),
                 new InputOption('step', '', InputOption::VALUE_NONE, 'Interactive execution. Ask user validation before every step.'),
                 new InputOption('report', '', InputOption::VALUE_NONE, 'Display profiles reports in the console output.'),
             ])

Player/Extension/BlackfireEnvResolver.php

@@ -35,9 +35,13 @@ public function __construct(
     /**
      * Resolves the environment from the current StepContext.
      *
-     * When the return value is false, it means that we shouldn't profile the current step.
+     * The return value tells the caller how to profile the current step:
+     *  - false:  don't profile the step;
+     *  - null:   profile the step without an explicit environment (the agent /
+     *            personal collab token decides where the profile lands);
+     *  - string: profile the step in that environment.
      */
-    public function resolve(StepContext $stepContext, ScenarioContext $scenarioContext, AbstractStep $step): string|false
+    public function resolve(StepContext $stepContext, ScenarioContext $scenarioContext, AbstractStep $step): string|false|null
     {
         // let's check if the current step resolves false. In that case, it means that we won't profile the step URL
         $env = $stepContext->getBlackfireEnv();
@@ -60,12 +64,11 @@ public function resolve(StepContext $stepContext, ScenarioContext $scenarioConte
         }
 
         // it resolved true: we'll profile the step using the default environment.
+        // When no default environment is set, we still profile the step but
+        // without an explicit environment (the agent / personal collab token
+        // decides where the profile lands).
         if (true === $env) {
-            if (null === $this->defaultEnv) {
-                throw new \LogicException('--blackfire-env option must be set when using "blackfire: true" in a scenario.');
-            }
-
-            $env = $this->defaultEnv;
+            return $this->defaultEnv;
         }
 
         return $env ?? false;

Player/Tests/Extension/BlackfireEnvResolverTest.php

@@ -26,7 +26,7 @@
 class BlackfireEnvResolverTest extends TestCase
 {
     #[DataProvider('resolveAnEnvironmentProvider')]
-    public function testResolveAnEnvironment(string|null $defaultEnv, string|null $stepBlackfire, string|null $scenarioSetEnvironment, string|bool $expectedResult): void
+    public function testResolveAnEnvironment(string|null $defaultEnv, string|null $stepBlackfire, string|null $scenarioSetEnvironment, string|bool|null $expectedResult): void
     {
         [
             $resolver,
@@ -70,6 +70,13 @@ public static function resolveAnEnvironmentProvider(): \Generator
             true,
         ];
 
+        yield 'resolves null when the blackfire step is true but no default env is set' => [
+            null,
+            'true',
+            null,
+            null,
+        ];
+
         yield 'resolves false when no env were found' => [
             null,
             null,
@@ -111,31 +118,6 @@ public static function precedenceProvider(): \Generator
         ];
     }
 
-    #[DataProvider('errorsProvider')]
-    public function testResolveThrowsAnError(string|null $defaultEnv, string|null $stepBlackfire, string|null $scenarioSetEnvironment): void
-    {
-        [
-            $resolver,
-            $stepContext,
-            $scenarioContext,
-            $step,
-        ] = $this->arrange($defaultEnv, $stepBlackfire, $scenarioSetEnvironment);
-
-        $this->expectException(\LogicException::class);
-        $this->expectExceptionMessage('--blackfire-env option must be set when using "blackfire: true" in a scenario.');
-
-        $resolver->resolve($stepContext, $scenarioContext, $step);
-    }
-
-    public static function errorsProvider(): \Generator
-    {
-        yield 'no env were found but step resolves true' => [
-            null,
-            'true',
-            null,
-        ];
-    }
-
     #[DataProvider('deprecationsProvider')]
     public function testDeprecationWarning(string|null $defaultEnv, string|null $stepBlackfire, string|null $scenarioSetEnvironment, string|bool $expectedResult, array $expectedDeprecations): void
     {

Player/Tests/Extension/BlackfireExtensionTest.php

@@ -172,38 +172,26 @@ static function (ScenarioContext $scenarioContext): void {
         ];
     }
 
-    #[DataProvider('beforeRequestFailureProvider')]
-    public function testBeforeRequestFailure(Step $step, HttpRequest $request, array $defaultScenarioSetExtraValues, string $exceptionClass, string $exceptionMessage): void
+    public function testBeforeRequestProfilesWhenBlackfireTrueWithoutDefaultEnv(): void
     {
+        // "blackfire: true" without a --blackfire-env must still profile the step:
+        // the agent / personal collab token decides where the profile lands.
         $extension = $this->getBlackfireExtension(null);
 
+        $step = new VisitStep('https://app-under-test.lan');
+        $step->blackfire('true');
+
         $stepContext = new StepContext();
         $stepContext->update($step, []);
 
         $scenarioContext = new ScenarioContext('"foo"', new ScenarioSet());
 
-        foreach ($defaultScenarioSetExtraValues as $k => $v) {
-            $scenarioContext->setExtraValue($k, $v);
-        }
-
-        $this->expectException($exceptionClass);
-        $this->expectExceptionMessage($exceptionMessage);
-
+        $request = new HttpRequest('GET', 'https://app-under-test.lan');
         $extension->beforeStep(new RequestStep($request, $step), $stepContext, $scenarioContext);
-    }
 
-    public static function beforeRequestFailureProvider(): \Generator
-    {
-        $defaultScenarioSetExtraValues = [];
-        $step = new VisitStep('https://app-under-test.lan');
-        $step->blackfire('true');
-        yield 'throws when blackfire env is true but not defined at the ScenarioContext level' => [
-            $step,
-            new HttpRequest('GET', 'https://app-under-test.lan'),
-            $defaultScenarioSetExtraValues,
-            \LogicException::class,
-            '--blackfire-env option must be set when using "blackfire: true" in a scenario.',
-        ];
+        $this->assertArrayHasKey(BlackfireExtension::HEADER_BLACKFIRE_QUERY, $request->headers);
+        $this->assertEquals(['1234'], $request->headers[BlackfireExtension::HEADER_BLACKFIRE_QUERY]);
+        $this->assertEquals(['1111-2222-3333-4444'], $request->headers[BlackfireExtension::HEADER_BLACKFIRE_PROFILE_UUID]);
     }
 
     #[DataProvider('afterResponseFailureProvider')]