You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
tests/lint/ fixtures are inside clang-tidy-diff's scope, and nothing tells the author that until CI is red
What happened
PR #644 added a seventh fixture directory under tests/lint/
(tests/lint/bidi_controls/, for #628's raw-bidi-control gate). Its three .cpp fixtures turned clang-tidy-diff red with 14 findings, every one of
them inside the new fixture files — job 106163759717, run 35542939709:
These files are text fixtures. Their own headers say so — "Nothing here is
compiled; the file exists to be read by the checker"
(tests/lint/catch_test_names/valid/ordinary_names.cpp), "Not compiled -- it
is scanned as text" (tests/lint/nolint_directives/valid/effective_directives.hpp).
None of them is in compile_commands.json. They are nevertheless analysed,
because:
clang-tidy-diff.py analyses every changed C/C++ line in the diff, and every
line of a new file is a changed line;
the clang-tidy job passes no-regex and no -exclude — deliberately,
and the step's own comment explains at length why (morph#481: an exclusion
there silently dropped 276 of the database's 690 entries);
there is no .clang-tidy anywhere under tests/lint/ — find . -name .clang-tidy returns the repository root, tests/, and the ten examples/*/tests/ copies, and nothing else;
clang tooling does not skip a file with no compile command; it interpolates
one from a neighbouring entry.
Why the first six directories did not hit this
They are green by accident of content, not by design. The 30 .cpp and 12 .hpp fixtures in nolint_directives, automoc_includes, catch_test_names, deprecated_markers, journal_stamps and test_type_names happen to declare
nothing clang-tidy objects to, and the checks that would have objected
(misc-use-internal-linkage, readability-identifier-length) are among the
thirteen tests/.clang-tidy subtracts — a file whose stated subject is Catch2
and raw-syscall idiom, which reaches these fixtures only because clang-tidy
resolves configuration from the translation unit's path.
So the convention that makes a tests/lint/ fixture safe — write it so
clang-tidy has nothing to say — is real, load-bearing, and written down
nowhere. The seventh directory is the first one to find out, and the eighth
will find out the same way.
Verification status
Reproduced, locally, on the PR branch, with clang-tidy 22.1.8 (CI's pinned CLANG_VERSION: "22"), using the warning set cmake/compiler_options.cmake
builds for Clang — -Weverything minus every suppression in its Clang branch.
That flag set reproduces CI's deduplicated list exactly: same 14 findings, same
files, same lines, same columns.
The absence of an exclusion is read from .github/workflows/ci.yml and
from find . -name .clang-tidy, not inferred.
Not verified: whether any of the six pre-existing fixture directories would
go red if their files were re-added to a diff today. I measured only the bidi_controls set. A cheap way to check would be to re-run clang-tidy-diff.py over git diff of a synthetic commit that touches one line
in each of the 42 existing fixtures.
Options
Not a proposal, a list — whoever picks this up should argue for one:
Write the convention down, in docs/spec/testing_strategy.md (which
already documents tests/lint/test_type_names/) and/or a short tests/lint/README.md: a fixture is scanned by clang-tidy-diff on the PR
that adds it, so it must be clang-tidy-clean; inline your namespace-scope
constants; suppress per line with the reason above the directive.
A gate that proves it, of the kind this repository prefers: run
clang-tidy over every file under tests/lint/ on every PR (not just changed
lines), so the seventh directory's failure mode is caught at the point the
fixture is written rather than at the point CI runs. Cost: it needs the clang-tidy job's full Qt + ladder configure, which already exists in that
job.
A tests/lint/.clang-tidy. Cheapest, and the one PR ci: reject raw bidi control characters in first-party files (fixes #628) #644 deliberately
did not take: it puts a new suppression file into the area morph#632
landed scripts/check_tidy_suppression_scope.sh to audit, and a
directory-wide -misc-misleading-bidirectional would have discarded real
coverage. If someone takes this route it should be argued check by check,
the way tests/.clang-tidy is.
Do nothing, and accept that each new fixture directory costs one red CI
round. Defensible — it has cost one so far — but it should then be a written
decision rather than a gap.
What would change the verdict
Close this if either (1) the convention is documented where a fixture author
will read it and the documentation is cited by something that fails when it
goes stale, or (2) a gate makes the failure impossible to reach CI. Re-open if a
new tests/lint/ directory turns clang-tidy-diff red again.
tests/lint/fixtures are insideclang-tidy-diff's scope, and nothing tells the author that until CI is redWhat happened
PR #644 added a seventh fixture directory under
tests/lint/(
tests/lint/bidi_controls/, for #628's raw-bidi-control gate). Its three.cppfixtures turnedclang-tidy-diffred with 14 findings, every one ofthem inside the new fixture files — job
106163759717, run35542939709:These files are text fixtures. Their own headers say so — "Nothing here is
compiled; the file exists to be read by the checker"
(
tests/lint/catch_test_names/valid/ordinary_names.cpp), "Not compiled -- itis scanned as text" (
tests/lint/nolint_directives/valid/effective_directives.hpp).None of them is in
compile_commands.json. They are nevertheless analysed,because:
clang-tidy-diff.pyanalyses every changed C/C++ line in the diff, and everyline of a new file is a changed line;
clang-tidyjob passes no-regexand no-exclude— deliberately,and the step's own comment explains at length why (morph#481: an exclusion
there silently dropped 276 of the database's 690 entries);
.clang-tidyanywhere undertests/lint/—find . -name .clang-tidyreturns the repository root,tests/, and the tenexamples/*/tests/copies, and nothing else;one from a neighbouring entry.
Why the first six directories did not hit this
They are green by accident of content, not by design. The 30
.cppand 12.hppfixtures innolint_directives,automoc_includes,catch_test_names,deprecated_markers,journal_stampsandtest_type_nameshappen to declarenothing clang-tidy objects to, and the checks that would have objected
(
misc-use-internal-linkage,readability-identifier-length) are among thethirteen
tests/.clang-tidysubtracts — a file whose stated subject is Catch2and raw-syscall idiom, which reaches these fixtures only because clang-tidy
resolves configuration from the translation unit's path.
So the convention that makes a
tests/lint/fixture safe — write it soclang-tidy has nothing to say — is real, load-bearing, and written down
nowhere. The seventh directory is the first one to find out, and the eighth
will find out the same way.
Verification status
Reproduced, locally, on the PR branch, with clang-tidy 22.1.8 (CI's pinned
CLANG_VERSION: "22"), using the warning setcmake/compiler_options.cmakebuilds for Clang —
-Weverythingminus every suppression in its Clang branch.That flag set reproduces CI's deduplicated list exactly: same 14 findings, same
files, same lines, same columns.
The absence of an exclusion is read from
.github/workflows/ci.ymlandfrom
find . -name .clang-tidy, not inferred.Not verified: whether any of the six pre-existing fixture directories would
go red if their files were re-added to a diff today. I measured only the
bidi_controlsset. A cheap way to check would be to re-runclang-tidy-diff.pyovergit diffof a synthetic commit that touches one linein each of the 42 existing fixtures.
Options
Not a proposal, a list — whoever picks this up should argue for one:
docs/spec/testing_strategy.md(whichalready documents
tests/lint/test_type_names/) and/or a shorttests/lint/README.md: a fixture is scanned byclang-tidy-diffon the PRthat adds it, so it must be clang-tidy-clean;
inlineyour namespace-scopeconstants; suppress per line with the reason above the directive.
clang-tidy over every file under
tests/lint/on every PR (not just changedlines), so the seventh directory's failure mode is caught at the point the
fixture is written rather than at the point CI runs. Cost: it needs the
clang-tidyjob's full Qt + ladder configure, which already exists in thatjob.
tests/lint/.clang-tidy. Cheapest, and the one PR ci: reject raw bidi control characters in first-party files (fixes #628) #644 deliberatelydid not take: it puts a new suppression file into the area morph#632
landed
scripts/check_tidy_suppression_scope.shto audit, and adirectory-wide
-misc-misleading-bidirectionalwould have discarded realcoverage. If someone takes this route it should be argued check by check,
the way
tests/.clang-tidyis.round. Defensible — it has cost one so far — but it should then be a written
decision rather than a gap.
What would change the verdict
Close this if either (1) the convention is documented where a fixture author
will read it and the documentation is cited by something that fails when it
goes stale, or (2) a gate makes the failure impossible to reach CI. Re-open if a
new
tests/lint/directory turnsclang-tidy-diffred again.Related
scripts/check_tidy_suppression_scope.sh, the reason option 3 is notfree.
clang-tidyjob carries no path exclusion, deliberately.