Skip to content

Commit 4206d9d

Browse files
lucatumelucatume
committed
fix: address review findings on parallel-run and Filesystem changes
recurseCopy's mirror() call now passes override and copy_on_windows so an existing destination tree is overwritten like cp -R/xcopy did. The MySQL reuse probe performs a PDO handshake with the configured credentials instead of a bare TCP connect, so a foreign listener on the port no longer counts as a running server; getInfo() uses the same probe and stop() guards against a missing PID file. ParallelRun drops the failed flag duplicating DotAggregator::hasFailures(). Controller tests stub PDO so no test depends on a live listener; the port-reuse branch gets a dedicated test.
1 parent 8b688cd commit 4206d9d

5 files changed

Lines changed: 63 additions & 19 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,17 @@ This project adheres to [Semantic Versioning](http://semver.org/).
77

88
### Fixed
99

10-
- `MysqlServerController` now probes the configured port before starting a server: a MySQL instance reachable on the port is reused instead of racing to start (and download) a second one. Fixes `parallel-run` workers, whose per-worker output dirs hid the shared server's PID file.
11-
- `parallel-run` prints `FAILURES!` instead of `OK` when a worker crashes before reporting results.
12-
- `parallel-run` resolves the managed MySQL data dir through `codecept_output_dir()` instead of a hardcoded `var/_output` path that ignored `paths.output` overrides and broke the 103-char unix socket path limit on deep checkouts.
10+
- `MysqlServerController` now probes the configured port with a PDO handshake before starting a server: a MySQL instance reachable on the port is reused instead of racing to start (and download) a second one. Fixes `parallel-run` workers, whose per-worker output dirs hid the shared server's PID file (#812).
11+
- `parallel-run` prints `FAILURES!` instead of `OK` when a worker crashes before reporting results (#812).
12+
- `parallel-run` resolves the managed MySQL data dir through `codecept_output_dir()` instead of a hardcoded `var/_output` path that ignored `paths.output` overrides and broke the 103-char unix socket path limit on deep checkouts (#812).
1313

1414
### Changed
1515

16-
- `Utils\Filesystem::rrmdir()` and `recurseCopy()` now delegate to the bundled `symfony/filesystem` component instead of a hand-rolled recursive delete and a `cp -R`/`xcopy` shell-out; public signatures and bool return contracts are unchanged.
16+
- `Utils\Filesystem::rrmdir()` and `recurseCopy()` now delegate to the bundled `symfony/filesystem` component instead of a hand-rolled recursive delete and a `cp -R`/`xcopy` shell-out; public signatures are unchanged, and failure paths that previously threw (unreadable dir, uncreatable destination) now return `false` like the other failure modes (#812).
1717

1818
### Removed
1919

20-
- Remove dead internal classes: `Utils\DockerCompose`, `Environment\Constants`, `Events\EventDispatcherException`, `Polyfills\Dotenv\Dotenv` and the `WordPress\CodeExecution` `ExitAction`/`ThrowAction` test doubles. No user-facing API is affected.
20+
- Remove dead internal classes: `Utils\DockerCompose`, `Environment\Constants`, `Events\EventDispatcherException`, `Polyfills\Dotenv\Dotenv` and the `WordPress\CodeExecution` `ExitAction`/`ThrowAction` test doubles. No user-facing API is affected (#812).
2121

2222
- Drop the v3.5 downgrade Rector harness (`config/rector-35.php`, `config/composer-35.json`, the `RemoveTypeHinting`/`DowngradePhpOsFamily`/`SerializableThrowableCompatibilityRector` rules, the `bin/build-35-*` build scripts, and the dead `build_35` Makefile target) from `master`; it now lives entirely on the v3.5 branch (#810).
2323

‎src/Command/ParallelRun.php‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,6 @@ public function execute(InputInterface $input, OutputInterface $output): int
102102

103103
$start = microtime(true);
104104

105-
$failed = false;
106105
$eventFiles = [];
107106
$eventOffsets = [];
108107
$logFiles = [];
@@ -175,7 +174,6 @@ public function execute(InputInterface $input, OutputInterface $output): int
175174
$this->drainEvents($eventFiles[$i], $eventOffsets, $i, $aggregator);
176175
if (!$p->isRunning()) {
177176
if (!$p->isSuccessful()) {
178-
$failed = true;
179177
$aggregator->recordCrash($i, $p->getExitCode() ?? -1);
180178
}
181179
unset($remaining[$i]);
@@ -206,7 +204,7 @@ public function execute(InputInterface $input, OutputInterface $output): int
206204
$this->teardownWorkers($workers, $cwd ?? '.', $output);
207205
}
208206

209-
return ($failed || $aggregator->hasFailures()) ? 1 : 0;
207+
return $aggregator->hasFailures() ? 1 : 0;
210208
}
211209

212210
/**

‎src/Extension/MysqlServerController.php‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ public function start(OutputInterface $output): void
2121
$pidFile = $this->getPidFile();
2222
$port = $this->getPort();
2323

24-
if ($this->isProcessRunning($pidFile) || $this->isPortAcceptingConnections($port)) {
24+
if ($this->isProcessRunning($pidFile) || $this->isMysqlServerReachable($port)) {
2525
$output->writeln("MySQL server already running on port $port.");
2626

2727
return;
@@ -57,17 +57,20 @@ public function getPidFile(): string
5757
return codecept_output_dir(self::PID_FILE_NAME);
5858
}
5959

60-
private function isPortAcceptingConnections(int $port): bool
60+
private function isMysqlServerReachable(int $port): bool
6161
{
62-
$socket = @stream_socket_client("tcp://127.0.0.1:{$port}", $errno, $errstr, 1);
62+
try {
63+
new \PDO(
64+
"mysql:host=127.0.0.1;port={$port}",
65+
$this->getUser(),
66+
$this->getPassword(),
67+
[\PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION, \PDO::ATTR_TIMEOUT => 1]
68+
);
6369

64-
if ($socket === false) {
70+
return true;
71+
} catch (\PDOException) {
6572
return false;
6673
}
67-
68-
fclose($socket);
69-
70-
return true;
7174
}
7275

7376
private function getDatabase(): string
@@ -140,7 +143,7 @@ public function getPort(): int
140143
public function stop(OutputInterface $output): void
141144
{
142145
$pidFile = $this->getPidFile();
143-
$mysqlServerPid = (int)file_get_contents($pidFile);
146+
$mysqlServerPid = is_file($pidFile) ? (int)file_get_contents($pidFile) : 0;
144147

145148
if (!$mysqlServerPid) {
146149
$output->writeln('MySQL server not running.');
@@ -168,7 +171,7 @@ public function getPrettyName(): string
168171
*/
169172
public function getInfo(): array
170173
{
171-
$isRunning = is_file($this->getPidFile());
174+
$isRunning = is_file($this->getPidFile()) || $this->isMysqlServerReachable($this->getPort());
172175

173176
$info = [
174177
'running' => $isRunning ? 'yes' : 'no',

‎src/Utils/Filesystem.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,12 @@ public static function recurseCopy(string $source, string $destination): bool
159159

160160
try {
161161
if (is_dir($resolvedSource)) {
162-
self::symfonyFilesystem()->mirror($resolvedSource, $destination);
162+
self::symfonyFilesystem()->mirror(
163+
$resolvedSource,
164+
$destination,
165+
null,
166+
['override' => true, 'copy_on_windows' => true]
167+
);
163168
} else {
164169
self::symfonyFilesystem()->copy(
165170
$resolvedSource,

‎tests/unit/lucatume/WPBrowser/Extension/MysqlServerControllerTest.php‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010
use lucatume\WPBrowser\Traits\UopzFunctions;
1111
use lucatume\WPBrowser\Utils\Download;
1212
use lucatume\WPBrowser\Utils\Filesystem as FS;
13+
use PDO;
14+
use PDOException;
1315
use PHPUnit\Framework\AssertionFailedError;
1416
use Symfony\Component\Console\Output\BufferedOutput;
1517
use Symfony\Component\Console\Output\NullOutput;
@@ -40,6 +42,13 @@ protected function _before()
4042
}
4143
]));
4244

45+
// No MySQL server is reachable unless a test says otherwise.
46+
$this->setClassMock(PDO::class, $this->makeEmptyClass(PDO::class, [
47+
'__construct' => function (): void {
48+
throw new PDOException('Connection refused');
49+
}
50+
]));
51+
4352
$pidFile = (new MysqlServerController([], []))->getPidFile();
4453
$this->setFunctionReturn('is_file', function (string $file) use ($pidFile): bool {
4554
return $file === $pidFile ? false : is_file($file);
@@ -392,6 +401,25 @@ public function testWillNotRestartIfAlreadyRunning(): void
392401
$controller->start(new NullOutput);
393402
}
394403

404+
public function testWillNotStartIfServerReachableOnPort(): void
405+
{
406+
// A PDO connection to the configured port succeeds: a MySQL server is already there.
407+
$this->setClassMock(PDO::class, $this->makeEmptyClass(PDO::class, []));
408+
$this->setClassMock(MysqlServer::class, $this->makeEmptyClass(MysqlServer::class, [
409+
'__construct' => function () {
410+
throw new AssertionFailedError(
411+
'The MysqlServer constructor should not be called.'
412+
);
413+
},
414+
]));
415+
416+
$controller = new MysqlServerController([], []);
417+
$output = new BufferedOutput();
418+
$controller->start($output);
419+
420+
$this->assertStringContainsString('already running', $output->fetch());
421+
}
422+
395423
public function testGetPort(): void
396424
{
397425
$controller = new MysqlServerController([
@@ -413,6 +441,10 @@ public function testStopRunningMysqlServer(): void
413441
])
414442
);
415443
$pidFile = (new MysqlServerController([], []))->getPidFile();
444+
$pidFileExists = false;
445+
$this->setFunctionReturn('is_file', function (string $file) use ($pidFile, &$pidFileExists): bool {
446+
return $file === $pidFile ? $pidFileExists : is_file($file);
447+
}, true);
416448
$this->setFunctionReturn('file_get_contents', function (string $file) use ($pidFile): string|false {
417449
if ($file === $pidFile) {
418450
return '12345';
@@ -434,6 +466,7 @@ public function testStopRunningMysqlServer(): void
434466

435467
$controller = new MysqlServerController($config, $options);
436468
$controller->start($output);
469+
$pidFileExists = true;
437470
$controller->stop($output);
438471
$port = MysqlServer::PORT_DEFAULT;
439472
$this->assertEquals(
@@ -489,6 +522,10 @@ public function testStopThrowsIfPidFileCannotBeUnlinked(): void
489522
])
490523
);
491524
$pidFile = (new MysqlServerController([], []))->getPidFile();
525+
$pidFileExists = false;
526+
$this->setFunctionReturn('is_file', function (string $file) use ($pidFile, &$pidFileExists): bool {
527+
return $file === $pidFile ? $pidFileExists : is_file($file);
528+
}, true);
492529
$this->setFunctionReturn('file_get_contents', function (string $file) use ($pidFile): string|false {
493530
if ($file === $pidFile) {
494531
return '12345';
@@ -504,6 +541,7 @@ public function testStopThrowsIfPidFileCannotBeUnlinked(): void
504541

505542
$controller = new MysqlServerController($config, $options);
506543
$controller->start($output);
544+
$pidFileExists = true;
507545
$this->expectException(ExtensionException::class);
508546
$this->expectExceptionMessage("Could not delete PID file '$pidFile'.");
509547
$controller->stop($output);

0 commit comments

Comments
 (0)