Skip to content

Commit 7193ef3

Browse files
author
Test
committed
fix(jobs): ScriptJob.getDisplayName returns job name, not executable (BUG-23)
ScriptJob overrode getDisplayName() to return ->executable instead of inheriting the parent's ->name. Two parallel jobs sharing the same (typical phpunit-shard pattern) printed identical OK/KO/SKIP lines in JobExecutor output, making them impossible to distinguish in the dashboard, dry-run and stats. Override was undocumented and inconsistent with every other Job type. Removed. - src/Jobs/ScriptJob.php: drop the override - tests/Unit/Jobs/JobAbstractTest.php: parametrize the display-name invariant test across all 13 Job types (regression net for future types) - tests/System/Release/ScriptJobDisplayNameReleaseTest.php: two @group release tests covering single-job and two-parallel-same-executable cases against the bundled .phar - docs/changelog.md: entry under [3.4.1] JSON v2 envelope unchanged (name/type come from getName/getType, not getDisplayName).
1 parent 30fc5fa commit 7193ef3

4 files changed

Lines changed: 150 additions & 5 deletions

File tree

‎docs/changelog.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ All notable changes to this project are documented here.
88

99
- **Unknown CLI options no longer silently shift the parser** in `flow`, `flows` and `job` (BUG-21). The three execution commands keep `ignoreValidationErrors()` on so that `job <name> -- <args>` keeps forwarding extra args to the underlying tool, but a typo such as `flow qa --foo=bar --config=/path/x.php` would previously make Symfony swallow `--foo` *and* silently drop `--config`, then fall back to `qa/githooks.php` — wrong config, wrong jobs, no error. A new `ValidatesUnknownOptionsBeforeDashDash` concern now inspects the input tokens before either command reads `--config`, rejects unknown long options and short shortcuts (cluster-aware), and emits a Symfony-style `The "--foo" option does not exist.` per offender; `flow` and `flows` additionally emit a custom error if `--` itself is present (neither command supports passthrough). The new behaviour caps any input-validation typo at exit 1 *before* the configuration file is resolved, so a typo can no longer accidentally execute the project-wide QA flow.
1010

11+
- **`script`-typed jobs now report their job key in OK/KO/SKIP logs instead of the executable path** (BUG-23). `ScriptJob::getDisplayName()` historically overrode the parent and returned `$this->executable`, so two parallel jobs of `type: script` sharing the same `executable-path` (e.g. two shards invoking the same `./run-tests` runner with different `other-arguments`) printed two identical `./run-tests - OK. Time: …` lines, making them indistinguishable in the dashboard, JSON v2 dry-run and stats. The override was undocumented and inconsistent with every other Job type (Phpstan, Phpunit, Phpcs, Phpmd, Psalm, ParallelLint, Rector, PhpCsFixer, Phpcpd, Paratest, Custom, all of which inherit `JobAbstract::getDisplayName()` returning `$this->name`). Removed. The JSON v2 envelope is unaffected — its `name`/`type` fields come from `getName()`/`getType()`, not `getDisplayName()`.
12+
1113
## [3.4]
1214

1315
### Added

‎src/Jobs/ScriptJob.php‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,4 @@ public function buildCommand(): string
2929

3030
return $command;
3131
}
32-
33-
public function getDisplayName(): string
34-
{
35-
return $this->executable;
36-
}
3732
}

‎tests/System/Release/ScriptJobDisplayNameReleaseTest.php‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Tests\System\Release;
6+
7+
use Tests\ReleaseTestCase;
8+
9+
/**
10+
* Release test for BUG-23: `ScriptJob::getDisplayName()` used to return
11+
* `$this->executable` instead of `$this->name`, making two parallel `script`-typed
12+
* jobs with the same executable indistinguishable in the OK/KO/SKIP output of
13+
* the executor. After the fix every Job type (including `script`) returns its
14+
* config key — the override in `src/Jobs/ScriptJob.php` is gone.
15+
*
16+
* Required as @group release because the consumer (`Wtyd\GitHooks\Execution\JobExecutor`)
17+
* lives inside the `.phar`; an asserted regression here means the fix is embedded
18+
* in the bundled binary.
19+
*
20+
* @group release
21+
*/
22+
class ScriptJobDisplayNameReleaseTest extends ReleaseTestCase
23+
{
24+
private string $configPath;
25+
26+
protected function setUp(): void
27+
{
28+
parent::setUp();
29+
30+
$this->configPath = self::TESTS_PATH . '/githooks.php';
31+
$this->configurationFileBuilder->enableV3Mode();
32+
}
33+
34+
/** @test */
35+
public function job_output_shows_job_name_not_executable_path_for_script_type(): void
36+
{
37+
$this->configurationFileBuilder
38+
->setV3Jobs([
39+
'shard_alpha' => [
40+
'type' => 'script',
41+
'executable-path' => '/bin/echo run-shared-tests',
42+
],
43+
]);
44+
45+
file_put_contents($this->configPath, $this->configurationFileBuilder->buildV3Php());
46+
47+
passthru("$this->githooks job shard_alpha --config=$this->configPath 2>&1", $exitCode);
48+
49+
$this->assertEquals(0, $exitCode);
50+
$output = $this->getActualOutput();
51+
// The success line is the canonical "name - OK. Time: X" emitted by JobExecutor.
52+
// Before the fix it was "/bin/echo run-shared-tests - OK. Time: X" — the executable
53+
// path bled into the log and shadowed the job key.
54+
$this->assertStringContainsString('shard_alpha - OK', $output);
55+
$this->assertStringNotContainsString('/bin/echo run-shared-tests - OK', $output);
56+
}
57+
58+
/**
59+
* The real-world case from the bug report: two parallel script jobs with the
60+
* same executable. Before the fix they were indistinguishable in the output;
61+
* after the fix each one reports its own job key.
62+
*
63+
* @test
64+
*/
65+
public function two_parallel_script_jobs_with_same_executable_are_distinguishable(): void
66+
{
67+
$this->configurationFileBuilder
68+
->setV3Flows(['shards' => ['jobs' => ['shard_a', 'shard_b']]])
69+
->setV3Jobs([
70+
'shard_a' => [
71+
'type' => 'script',
72+
'executable-path' => '/bin/echo identical-runner',
73+
],
74+
'shard_b' => [
75+
'type' => 'script',
76+
'executable-path' => '/bin/echo identical-runner',
77+
],
78+
]);
79+
80+
file_put_contents($this->configPath, $this->configurationFileBuilder->buildV3Php());
81+
82+
passthru("$this->githooks flow shards --config=$this->configPath --processes=2 2>&1", $exitCode);
83+
84+
$this->assertEquals(0, $exitCode);
85+
$output = $this->getActualOutput();
86+
$this->assertStringContainsString('shard_a - OK', $output);
87+
$this->assertStringContainsString('shard_b - OK', $output);
88+
// The pre-fix output would have shown "/bin/echo identical-runner - OK" twice.
89+
$this->assertLessThanOrEqual(
90+
1,
91+
substr_count($output, '/bin/echo identical-runner - OK'),
92+
'Two parallel script jobs were shown with their executable instead of their job key.'
93+
);
94+
}
95+
}

‎tests/Unit/Jobs/JobAbstractTest.php‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,15 @@
1010
use Wtyd\GitHooks\Jobs\ParallelLintJob;
1111
use Wtyd\GitHooks\Jobs\ParatestJob;
1212
use Wtyd\GitHooks\Jobs\PhpcbfJob;
13+
use Wtyd\GitHooks\Jobs\PhpcpdJob;
14+
use Wtyd\GitHooks\Jobs\PhpCsFixerJob;
1315
use Wtyd\GitHooks\Jobs\PhpcsJob;
1416
use Wtyd\GitHooks\Jobs\PhpmdJob;
1517
use Wtyd\GitHooks\Jobs\PhpstanJob;
18+
use Wtyd\GitHooks\Jobs\PhpunitJob;
1619
use Wtyd\GitHooks\Jobs\PsalmJob;
20+
use Wtyd\GitHooks\Jobs\RectorJob;
21+
use Wtyd\GitHooks\Jobs\ScriptJob;
1722

1823
/**
1924
* Direct coverage for JobAbstract logic that was only exercised indirectly.
@@ -101,6 +106,54 @@ public function name_type_and_display_name_are_exposed_verbatim()
101106
$this->assertSame('phpstan_src', $job->getDisplayName());
102107
}
103108

109+
/**
110+
* BUG-23 regression net: `getDisplayName()` must return the job key for every Job type,
111+
* not the executable. A `script`-typed override historically returned `$this->executable`,
112+
* making two parallel jobs with the same executable indistinguishable in OK/KO/SKIP logs.
113+
* Parametrized so adding a new Job type without inheriting the parent behaviour breaks here.
114+
*
115+
* @test
116+
* @dataProvider allJobClassesProvider
117+
*/
118+
public function display_name_returns_job_key_for_every_job_type(
119+
string $jobClass,
120+
string $jobType
121+
): void {
122+
$config = new JobConfiguration(
123+
"{$jobType}_shard_a",
124+
$jobType,
125+
[
126+
'executable-path' => './run-tests',
127+
'script' => 'true',
128+
'paths' => ['src'],
129+
]
130+
);
131+
132+
$job = new $jobClass($config);
133+
134+
$this->assertSame("{$jobType}_shard_a", $job->getDisplayName());
135+
}
136+
137+
/** @return array<string, array{0: class-string, 1: string}> */
138+
public static function allJobClassesProvider(): array
139+
{
140+
return [
141+
'phpstan' => [PhpstanJob::class, 'phpstan'],
142+
'phpmd' => [PhpmdJob::class, 'phpmd'],
143+
'phpcs' => [PhpcsJob::class, 'phpcs'],
144+
'phpcbf' => [PhpcbfJob::class, 'phpcbf'],
145+
'phpunit' => [PhpunitJob::class, 'phpunit'],
146+
'paratest' => [ParatestJob::class, 'paratest'],
147+
'psalm' => [PsalmJob::class, 'psalm'],
148+
'parallel-lint' => [ParallelLintJob::class, 'parallel-lint'],
149+
'phpcpd' => [PhpcpdJob::class, 'phpcpd'],
150+
'php-cs-fixer' => [PhpCsFixerJob::class, 'php-cs-fixer'],
151+
'rector' => [RectorJob::class, 'rector'],
152+
'script' => [ScriptJob::class, 'script'],
153+
'custom' => [CustomJob::class, 'custom'],
154+
];
155+
}
156+
104157
/** @test */
105158
public function ignore_errors_and_fail_fast_default_to_false()
106159
{

0 commit comments

Comments
 (0)