Skip to content

fix: static tests never detect conflict markers that were committed - #77

Open
eastagiletracker wants to merge 1 commit into
fourkitchens:mainfrom
eastagiletracker:agile-board/static-tests-detect-committed-conflict-markers
Open

fix: static tests never detect conflict markers that were committed#77
eastagiletracker wants to merge 1 commit into
fourkitchens:mainfrom
eastagiletracker:agile-board/static-tests-detect-committed-conflict-markers

Conversation

@eastagiletracker

Copy link
Copy Markdown

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 --check with no revision arguments, and that form only inspects changes which have not been committed yet. The static_tests job runs setup-project — a checkout plus dependency installs — so the tree it tests is a clean checkout and git diff --check has nothing to look at. The loop body therefore never runs, FAIL is never set, and test-results/git-merge-marker.xml always 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_SCRIPTS only reaches modules/custom, themes/custom and sites, so a conflict block in config/sync travels untouched all the way to drush cim on deploy.

Reproduced on main at bf496b2, against a small Drupal-shaped project whose committed config/sync/system.site.yml holds a conflict block:

$ git status --porcelain          # clean checkout, as in CI
$ git diff --check                # what the scan runs today — no output
$ DOCROOT=web bash ../pots/scripts/test/static
Checking for left over conflict markers.
Running PHP Lint
No syntax errors detected in web/modules/custom/example/example.module
Done
$ echo $?
0
$ cat test-results/git-merge-marker.xml
<?xml version="1.0" encoding="utf-8"?> <testsuites>
</testsuites>

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 --check rule also treats a bare ======= as a marker, and that fires on clean projects — a Markdown heading underlined with exactly seven = characters is reported as README.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. vendor and node_modules are excluded, matching what the PHP lint below already does.

Verification

Same fixture, after the change:

$ DOCROOT=web bash ../pots/scripts/test/static
Checking for left over conflict markers.
Merge conflict marker detected: config/sync/system.site.yml:3
Merge conflict marker detected: config/sync/system.site.yml:7
Running PHP Lint
No syntax errors detected in web/modules/custom/example/example.module
Tests Failed
$ echo $?
1

Four cases were exercised, each run against both the current script and the changed one:

  • committed markers in a config/sync YAML file — was exit 0 with an empty report, now exit 1 with two JUnit entries;
  • a clean project carrying a seven-character ======= heading underline — exit 0 both before and after, no false failure;
  • a marker present only as an uncommitted working-tree edit — still detected, so nothing the old form caught is lost;
  • a marker inside a committed vendor/ fixture file — ignored.

bash -n over all 17 scripts under scripts/ is unchanged. There is no test harness here to add a case to, so the fixture is the whole recipe: git init a directory holding composer.json, web/modules/custom, web/themes/custom and web/sites, commit a config/sync/*.yml containing a conflict block, then run the script with DOCROOT=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.

board

If you'd rather not receive contributions like this, reply no-more-prs on 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

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add checks for Merge conflicts left in code in linting.

1 participant