Runner reliability: waitForPageLoaded, exit codes, env, bin, --suites - #63
Merged
Merged
Conversation
…owing typos waitForPageLoaded() called $this->waitForNavigation(), which CommonPage does not define. __call only forwards to the chrome-php Page, which does not have it either (it lives on PageNavigation), and it dropped anything it could not forward. So the method returned immediately and every caller believed the page was loaded. goToProduct() and the "[Debug] This page has moved" fallback in FrontOfficePage::goToPage() made the same dead call. waitForPageLoaded() now polls document.readyState through waitForJsCondition() until the DOM is parsed and has a body, takes an optional timeout (30s by default) and throws TimeoutException when it runs out. The dead calls now go through it. __call now throws BadMethodCallException for a name neither the page object nor the chrome-php Page defines, so the next such mistake fails the step instead of passing silently. It also returns what the proxied method returned rather than null. With no browser page yet, a genuine chrome-php method is still a no-op, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bin/prestaflow listened to ConsoleEvents::ERROR to print its ERROR/TRACE lines, then called setExitCode(Command::SUCCESS). Symfony treats an exit code of 0 from that event as "handled" and drops the error, so a run that could not even start (Chrome failing to launch, a suites path that does not exist, a mistyped command) ended with status 0 and turned the CI job green. The application now lives in PrestaFlow\Library\Console\Application, which the binary merely runs, so its exit codes can be tested. The ERROR/TRACE rendering moved into doRenderThrowable(); Symfony's own handling then turns the error into exit code 1 (or the exception's code). catchErrors is on, because most failures here are \Error and would otherwise escape as a PHP fatal. Assertion failures still exit 1 through ExecuteSuite's return value, and a clean run still exits 0. The ERROR/TRACE lines now go to stderr, where Symfony writes rendered errors. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t too presetExtraHeadersFromEnv() read $_ENV directly. When PHP runs with a variables_order without "E", the usual case on CI, a variable set by the runner (a GitHub Actions env: block, for instance) never reaches $_ENV, so the headers were silently dropped: a WAF bypass header configured in CI did nothing and the run hit the challenge page instead. It now goes through Env::get(), like every other PRESTAFLOW_* read: $_ENV first (values loaded from .env), then getenv(). It was the last direct read of a PRESTAFLOW_* variable in src/; the remaining $_ENV writes in TestsSuite normalise booleans after reading through Env. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
composer.json had no "bin" key, so installing the library as a dependency never created vendor/bin/prestaflow: the command every guide tells users to run did not exist, and projects had to call the script inside vendor/prestaflow/php-library/bin themselves. bin/prestaflow already found the right autoloader in both installed layouts, through $_composer_autoload_path (set by Composer's vendor/bin proxy) and through ../../../autoload.php (direct call inside vendor/). The new test pins that down for both, running from an unrelated directory so the getcwd() fallback cannot be what makes them pass. Checked by hand too, with a throwaway project requiring the library from a path repository. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…SUITES The GitHub Action turns its `suites` input into PRESTAFLOW_SUITES, but the library never read it: a workflow asking for BackOffice ran every suite. `prestaflow run <path>` now takes the comma-separated names from --suites, or from PRESTAFLOW_SUITES (read through Env::get) when the option is absent, as sub-folders of <path>. Nested paths such as FrontOffice/Checkout are allowed, each folder is scanned recursively as before, and the run takes the union without duplicates. --group and --draft then apply to what is left. A name that matches no folder fails the run with the missing names and the available sub-folders: a typo in a workflow must not become a green job that ran zero tests. Absolute paths and ".." are refused, since the filter is only meant to narrow <path>. Combining a filter with a single suite file is an error for the same reason. Unset or empty, nothing changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A --suites / PRESTAFLOW_SUITES filter naming a folder that exists but holds no suite still ended with "Tests folder is empty" and exit code 0. Asking for specific suites and running none is a mistake in the filter, not an empty project, and on CI it shows up as a green job that tested nothing. When a filter is set and the run ends up with zero suites, whether the folders were empty or --group / --draft removed everything, it now stops with "Suites filter [...] selected no suite under [...]" and exit code 1. Without a filter an empty folder still exits 0. The filter's test fixtures now declare a namespace. Bare "<?php" files hit an existing "Undefined array key 0" warning in ExecuteSuite's namespace parsing, which this change does not address. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defects found while reviewing the PrestaFlow article series against the library. Each commit fixes one defect and adds a test that fails without the fix.
OK (325 tests, 713 assertions).b4fc41ewaitForPageLoaded()called awaitForNavigation()that exists nowhere, so it did nothing. It now waits fordocument.readyStatewith a timeout.__callthrowsBadMethodCallExceptionon an unknown method instead of swallowing typos.5bec86941206d2PRESTAFLOW_EXTRA_HEADERSwas read from$_ENVonly, and ignored undervariables_order=GPCS(setup-php's default). It now goes throughEnv::get().1562b92composer.jsondeclares"bin": ["bin/prestaflow"], sovendor/bin/prestaflowexists.46149a2--suites=A,B/PRESTAFLOW_SUITES(set by the GitHub Action'ssuitesinput, which had no effect) now runs only those sub-folders of the tests folder. Unknown names,..and absolute paths fail explicitly.43c9521Visible changes
--versionreports the version.waitForPageLoaded()can throwTimeoutException.Not included
The local commit
4e834c4and uncommitted work ondevare not in this branch.Found but not fixed:
namespacetriggers an "Undefined array key 0" warning inExecuteSuite;datas/is missing from the published package, so every browser start warns.🤖 Generated with Claude Code