fix: guard each screenshot directory the way the parent is guarded - #250
Open
yeapea wants to merge 1 commit into
Open
fix: guard each screenshot directory the way the parent is guarded#250yeapea wants to merge 1 commit into
yeapea wants to merge 1 commit into
Conversation
cleanup() protects the parent with is_dir() and then rmdir()s three children unguarded. Two of them, Sliders and ImageDiffView, belong to the visual-diff features and a plain run never creates them. The @ silences the message, but PHPUnit still records a suppressed diagnostic as a Test-Runner-Triggered PHP Warning, so a suite running with failOnWarning exits non-zero while every test passes. glob() is why the existing guard does not catch it: on a missing directory it answers with an empty array rather than false, so is_array() is true, the unlink loop runs over nothing, and control reaches the rmdir that warns.
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
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.
Screenshot::cleanup()guards the parent directory withis_dir()and then callsrmdir()onthree children without one. Two of those —
SlidersandImageDiffView— belong to thevisual-diff features, so an ordinary run never creates them.
The
@silences the message, but PHPUnit still records a suppressed diagnostic as aTest-Runner-Triggered PHP Warning. A suite running with
failOnWarning="true"therefore exitsnon-zero while every test passes:
Why the existing guard does not catch it
glob()on a missing directory answers with an empty array, notfalse. Sois_array($files)is true, the unlink loop runs over nothing, and control reaches the
rmdir()that warns.Why it is easy to miss
cleanup()early-returns while the screenshots directory does not exist. So the first run in afresh tree is green and only the second one warns — the first run creates the directory that
makes the next one warn. CI that checks out a clean workspace per run never sees it; running the
browser suite twice in a row locally reproduces it every time.
The change
Each directory is guarded the way the parent already is. It is additive and cannot alter behaviour
for a directory that exists — the loop body is unchanged for those.
Verified, both arms
On PHP 8.5 with
failOnWarning="true", running the browser suite twice in a row:Note
#229 also touches this file (configurable screenshot directory). The two do not overlap in intent —
this one only adds the missing guard inside the existing loop — but whichever lands second will want
a trivial rebase.