fix: static tests never detect conflict markers that were committed - #77
Open
eastagiletracker wants to merge 1 commit into
Conversation
The conflict marker scan ran `git diff --check` with no revisions, which only reports on changes that have not been committed yet. Static tests run against a clean checkout, so there was never anything for it to look at and the report always came back empty. Grep the tracked files instead, which covers committed content and still picks up uncommitted edits when the script is run locally. Matches are limited to `<<<<<<<` and `>>>>>>>`; a bare `=======` is a heading underline often enough that including it would fail builds on clean projects. vendor and node_modules are left out, as the PHP lint below already does. Offending paths are echoed to the job log so the failure is diagnosable without opening the JUnit artifact.
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.
This PR proposes fixing the leftover conflict marker scan in
scripts/test/static, which has never been able to report a marker that was already committed (Fixes #39). We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/286. You can sign in with your GitHub ID to claim ownership of the project.The defect
The scan calls
git diff --checkwith no revision arguments, and that form only inspects changes which have not been committed yet. Thestatic_testsjob runssetup-project— acheckoutplus dependency installs — so the tree it tests is a clean checkout andgit diff --checkhas nothing to look at. The loop body therefore never runs,FAILis never set, andtest-results/git-merge-marker.xmlalways comes back empty. That is exactly the case #39 was opened for: a commit carrying conflict markers passes static tests.PHP lint does not cover the gap. Markers most often land in YAML, and
$PHP_SCRIPTSonly reachesmodules/custom,themes/customandsites, so a conflict block inconfig/synctravels untouched all the way todrush cimon deploy.Reproduced on
mainat bf496b2, against a small Drupal-shaped project whose committedconfig/sync/system.site.ymlholds a conflict block:The change
The scan now greps the tracked files themselves, which covers committed content and still picks up uncommitted edits when the script is run locally. The JUnit format is untouched, and offending paths are echoed to the job log so a failure is diagnosable without opening the stored artifact.
Only
<<<<<<<and>>>>>>>are matched, which is deliberate. Git's own--checkrule also treats a bare=======as a marker, and that fires on clean projects — a Markdown heading underlined with exactly seven=characters is reported asREADME.md:2: leftover conflict marker. A genuine conflict always leaves the other two markers behind, so narrowing the match costs no detection and keeps the job from failing builds that are fine.vendorandnode_modulesare excluded, matching what the PHP lint below already does.Verification
Same fixture, after the change:
Four cases were exercised, each run against both the current script and the changed one:
config/syncYAML file — was exit 0 with an empty report, now exit 1 with two JUnit entries;=======heading underline — exit 0 both before and after, no false failure;vendor/fixture file — ignored.bash -nover all 17 scripts underscripts/is unchanged. There is no test harness here to add a case to, so the fixture is the whole recipe:git inita directory holdingcomposer.json,web/modules/custom,web/themes/customandweb/sites, commit aconfig/sync/*.ymlcontaining a conflict block, then run the script withDOCROOT=web.How this was managed
Your issues and pull requests were imported onto a board — 73 stories and 11 labels — and this work was tracked on the story for issue #39, at https://eastagiletracker.com/projects/286/stories/176752. The board itself is at https://eastagiletracker.com/projects/286.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com