85f69038d8
Four gates that were not gating. NO MIGRATION COLUMN NAME HAD EVER BEEN CHECKED. The naming gate matched db.Column attribute assignments only, so every column a migration creates has bypassed the lowercase-concatenated rule since the gate was written. It now reads migrations too, in the shapes they are actually written: sa.Column in create_table and add_column, alter_column's new_column_name, the batch equivalents, the keyword form, and a literal with a leading capital or a string prefix. Run against the tree it finds nothing, which is now a fact rather than an artefact of the pattern. THE ADR-015 SITE-LITERAL SCAN DID NOT REACH THE C# AGENT, twice over: agent/ was not in the directory list, and a file-level --exclude alongside the --include list changes which files grep matches at all, so the .cs include was not doing what it looked like it was doing. A hostname or base URL compiled into the agent would have shipped to another site with no CI signal. NO PLUGIN MIGRATION WAS CHECKED FOR TOUCHING SOMEBODY ELSE'S TABLES. ADR-008 says plugin chains own plugin schema and the core chain is reserved for core tables; the endpoints chain broke that, added and then dropped columns on the core assets table, and nothing would have stopped the next one. The scan walks every plugin chain, resolves each DDL op to its table, and fails when that table belongs to core or to another plugin - including through op.execute and through a bind, which is this repo's dominant idiom and the shape a check keyed on the receiver name would have missed entirely. Statements are classified one at a time, so a leading DML verb cannot hide trailing DDL. The two historical crossings are allowlisted by plugin, file, op, table AND column, so the shipped history stays green and anything new goes red. AND THE GATE RUNNER ITSELF PASSED WITHOUT RUNNING ANYTHING. In a checkout with no venv, `python` is often not on PATH; the pytest line ends in `|| true`, so the run died 127, the report was one command-not-found line, every gate matched nothing, and it still printed "All gates ran." and exited 0. That exit code is what everything else trusts. It now refuses to run without an interpreter that can import pytest. Recorded rather than fixed, with the reasoning in both files: the publishability scrub keys on docs/*.md, so prose that lives only in docs/wiki-drafts/ does not turn it on. Whether those drafts are internal or headed for publication is a question about what they are for, not a bug to close by rewriting them.
296 lines
12 KiB
Python
296 lines
12 KiB
Python
"""Holes in `scripts/check-naming-and-style.sh`, each held open by a test.
|
|
|
|
The naming gate must read the column names migrations CREATE, not just models.
|
|
|
|
`scripts/check-naming-and-style.sh` checked `db.Column` attribute names on
|
|
models and nothing else, while `migrations/versions/` is filtered out of the
|
|
file list every other check in that script uses. A column is born in a
|
|
migration, so from the day the gate was written no created column name was ever
|
|
looked at: a model attribute reading fine says nothing about what the database
|
|
actually got.
|
|
|
|
Two tests, and the second is the one that matters. A gate that prints nothing
|
|
looks identical whether it is clean or broken, and this one was silently
|
|
passing everything for its whole life, so the test plants a violation and
|
|
requires the gate to fail on it. Without that, a future edit that breaks the
|
|
regex would leave every test here green.
|
|
"""
|
|
|
|
import re
|
|
import shutil
|
|
import subprocess
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
REPO_ROOT = Path(__file__).resolve().parent.parent
|
|
GATE = REPO_ROOT / 'scripts' / 'check-naming-and-style.sh'
|
|
|
|
# Same shapes the gate matches: the name literal of a created column, positional
|
|
# or keyword, on the sa.Column( line or on the line after it, plus the target of
|
|
# a rename.
|
|
COLUMN_LITERAL = re.compile(r"""sa\.Column\(\s*['"]([^'"]+)['"]""", re.S)
|
|
COLUMN_KEYWORD = re.compile(
|
|
r"""sa\.Column\(\s*name\s*=\s*['"]([^'"]+)['"]""", re.S)
|
|
RENAME_LITERAL = re.compile(r"""new_column_name\s*=\s*['"]([^'"]+)['"]""")
|
|
|
|
# THE RULE, not a proxy for it. The first version of this file asked whether the
|
|
# name contained an underscore, which is the same mistake the gate's own regex
|
|
# made: 'Monitor_Name' has one but was missed for its leading capital, and a
|
|
# CamelCase name with no underscore at all is just as wrong and has nothing to
|
|
# find. Lowercase concatenated means lowercase concatenated.
|
|
LOWERCASE_CONCATENATED = re.compile(r'[a-z][a-z0-9]*\Z')
|
|
|
|
PLANTED = '''"""Planted by the test suite. Never applied."""
|
|
import sqlalchemy as sa
|
|
from alembic import op
|
|
|
|
revision = 'plantedprobe'
|
|
down_revision = None
|
|
|
|
|
|
def upgrade():
|
|
op.add_column('widgets', sa.Column('planted_column', sa.Integer(),
|
|
nullable=True))
|
|
|
|
|
|
def downgrade():
|
|
op.drop_column('widgets', 'planted_column')
|
|
'''
|
|
|
|
# ASSEMBLED FROM FRAGMENTS, like the gate's own patterns and
|
|
# tests/test_docs_publishable.py: written literally, a test for the site-literal
|
|
# rule is itself a site literal, and both the gate and the publication scrub
|
|
# would find it here.
|
|
PLANTED_SITE_HOST = 'tsg' 'wp00525' '.geaero' 'space.net'
|
|
PLANTED_AGENT = (
|
|
'// Planted by the test suite.\n'
|
|
'internal static class Probe\n'
|
|
'{\n'
|
|
' public const string DefaultBaseUrl = "https://'
|
|
+ PLANTED_SITE_HOST + '/shopdb";\n'
|
|
'}\n'
|
|
)
|
|
|
|
|
|
def _migration_files():
|
|
found = list((REPO_ROOT / 'migrations' / 'versions').glob('*.py'))
|
|
found += list(REPO_ROOT.glob('plugins/*/migrations/versions/*.py'))
|
|
return sorted(found)
|
|
|
|
|
|
def test_there_are_migrations_to_check():
|
|
"""Guard the guard: a glob that matches nothing passes everything below."""
|
|
assert _migration_files(), 'no migrations found - has the layout moved?'
|
|
|
|
|
|
def test_no_migration_creates_a_badly_named_column():
|
|
offenders = []
|
|
for path in _migration_files():
|
|
text = path.read_text(encoding='utf-8')
|
|
for pattern in (COLUMN_LITERAL, COLUMN_KEYWORD, RENAME_LITERAL):
|
|
for name in pattern.findall(text):
|
|
if not LOWERCASE_CONCATENATED.match(name):
|
|
offenders.append('{0}: {1}'.format(
|
|
path.relative_to(REPO_ROOT), name))
|
|
assert not offenders, (
|
|
'column names are lowercase concatenated (CONTRIBUTING.md):\n '
|
|
+ '\n '.join(offenders))
|
|
|
|
|
|
@pytest.mark.skipif(shutil.which('bash') is None, reason='needs bash')
|
|
def test_the_gate_fails_on_a_planted_migration_column(tmp_path):
|
|
"""Plant one, and require the gate to say so.
|
|
|
|
Run against a throwaway tree rather than the repository, so the planted
|
|
file cannot be left behind by a failing assert. tmp_path is outside any
|
|
checkout, so the script's git file list is unavailable and it takes its
|
|
own find fallback - which is the branch a developer without git metadata
|
|
gets, and so is worth exercising.
|
|
"""
|
|
versions = tmp_path / 'migrations' / 'versions'
|
|
versions.mkdir(parents=True)
|
|
gate = tmp_path / 'scripts' / 'check-naming-and-style.sh'
|
|
gate.parent.mkdir(parents=True)
|
|
shutil.copy(GATE, gate)
|
|
|
|
planted = versions / 'zz01_planted.py'
|
|
planted.write_text(PLANTED, encoding='utf-8')
|
|
dirty = subprocess.run(['bash', str(gate)], cwd=str(tmp_path),
|
|
capture_output=True, text=True)
|
|
assert dirty.returncode == 1, (
|
|
'the gate passed a snake_case migration column:\n' + dirty.stdout)
|
|
assert 'planted_column' in dirty.stdout
|
|
|
|
planted.unlink()
|
|
clean = subprocess.run(['bash', str(gate)], cwd=str(tmp_path),
|
|
capture_output=True, text=True)
|
|
assert clean.returncode == 0, (
|
|
'the gate fails with nothing planted:\n' + clean.stdout)
|
|
|
|
|
|
@pytest.mark.skipif(shutil.which('bash') is None, reason='needs bash')
|
|
def test_the_gate_fails_on_a_site_literal_in_the_agent(tmp_path):
|
|
"""agent/ is in the ADR-015 scan, and has to stay in it.
|
|
|
|
The C# agent is compiled once and installed on every bay at every site. A
|
|
site hostname, base URL or pin baked into a .cs file there ships to the next
|
|
site inside a binary, where no reviewer reads it - and agent/ was not one of
|
|
the directories the site-literal scan looked at.
|
|
"""
|
|
gate = tmp_path / 'scripts' / 'check-naming-and-style.sh'
|
|
gate.parent.mkdir(parents=True)
|
|
shutil.copy(GATE, gate)
|
|
agent = tmp_path / 'agent' / 'Probe'
|
|
agent.mkdir(parents=True)
|
|
|
|
planted = agent / 'Probe.cs'
|
|
planted.write_text(PLANTED_AGENT, encoding='utf-8')
|
|
dirty = subprocess.run(['bash', str(gate)], cwd=str(tmp_path),
|
|
capture_output=True, text=True)
|
|
assert dirty.returncode == 1, (
|
|
'the gate passed a site hostname in the agent:\n' + dirty.stdout)
|
|
assert 'Probe.cs' in dirty.stdout
|
|
|
|
planted.unlink()
|
|
clean = subprocess.run(['bash', str(gate)], cwd=str(tmp_path),
|
|
capture_output=True, text=True)
|
|
assert clean.returncode == 0, (
|
|
'the gate fails with nothing planted:\n' + clean.stdout)
|
|
|
|
|
|
# Every shape a migration writes a column name in. The gate's first regex asked
|
|
# for ['"][a-z0-9]*_ inside the quotes, so only the first of these was ever
|
|
# caught: a leading capital, a keyword argument or a CamelCase name with no
|
|
# underscore all walked past it. One case per shape, named so a failure says
|
|
# which shape regressed.
|
|
PLANTED_COLUMN_SHAPES = {
|
|
'positional_snake': (
|
|
" op.add_column('widgets', sa.Column('planted_column',\n"
|
|
" sa.Integer()))\n", 'planted_column'),
|
|
'positional_leading_capital': (
|
|
" op.add_column('widgets', sa.Column('Monitor_Name',\n"
|
|
" sa.String(50)))\n", 'Monitor_Name'),
|
|
'positional_camelcase_no_underscore': (
|
|
" op.add_column('widgets', sa.Column('MonitorName', sa.String(50)))\n",
|
|
'MonitorName'),
|
|
'keyword_name': (
|
|
" op.add_column('widgets', sa.Column(name='kw_form',\n"
|
|
" type_=sa.String()))\n", 'kw_form'),
|
|
'wrapped_after_open_paren': (
|
|
" op.add_column('widgets', sa.Column(\n"
|
|
" 'wrapped_name', sa.Integer()))\n", 'wrapped_name'),
|
|
'alter_column_rename': (
|
|
" op.alter_column('widgets', 'oldname',\n"
|
|
" new_column_name='New_Name')\n", 'New_Name'),
|
|
'create_table_second_column': (
|
|
" op.create_table('t2', sa.Column('goodone', sa.Integer()),\n"
|
|
" sa.Column('Bad_Two', sa.Integer()))\n", 'Bad_Two'),
|
|
'batch_alter_table_add': (
|
|
" with op.batch_alter_table('widgets') as batch_op:\n"
|
|
" batch_op.add_column(sa.Column('batchCol', sa.Integer()))\n",
|
|
'batchCol'),
|
|
'batch_alter_table_rename': (
|
|
" with op.batch_alter_table('widgets') as batch_op:\n"
|
|
" batch_op.alter_column('a', new_column_name='b_c')\n", 'b_c'),
|
|
}
|
|
|
|
PLANTED_CLEAN_BODY = (
|
|
" op.add_column('widgets', sa.Column('goodname', sa.Integer()))\n"
|
|
" op.add_column('widgets', sa.Column(name='alsogood', type_=sa.String()))\n"
|
|
" op.alter_column('widgets', 'oldname', new_column_name='newname')\n"
|
|
# drop_column names a column that already exists, so a bad name here is
|
|
# deliberately NOT a finding: it was wrong where it was created, and
|
|
# reporting it again makes one rename look like two.
|
|
" op.drop_column('widgets', 'legacy_old')\n"
|
|
)
|
|
|
|
|
|
def _migration_module(body):
|
|
return ('"""Planted by the test suite. Never applied."""\n'
|
|
'import sqlalchemy as sa\n'
|
|
'from alembic import op\n\n'
|
|
"revision = 'plantedprobe'\n"
|
|
'down_revision = None\n\n\n'
|
|
'def upgrade():\n' + body + '\n\n'
|
|
'def downgrade():\n'
|
|
' pass\n')
|
|
|
|
|
|
def _gate_tree(tmp_path):
|
|
gate = tmp_path / 'scripts' / 'check-naming-and-style.sh'
|
|
gate.parent.mkdir(parents=True, exist_ok=True)
|
|
shutil.copy(GATE, gate)
|
|
return gate
|
|
|
|
|
|
@pytest.mark.skipif(shutil.which('bash') is None, reason='needs bash')
|
|
@pytest.mark.parametrize('shape', sorted(PLANTED_COLUMN_SHAPES))
|
|
def test_the_gate_catches_every_column_shape(tmp_path, shape):
|
|
"""Plant one shape at a time and require the gate to name it.
|
|
|
|
Parametrised rather than one file holding all nine, so a regression says
|
|
WHICH shape stopped being seen. A single file would fail on the first one
|
|
and hide the rest.
|
|
"""
|
|
body, expected = PLANTED_COLUMN_SHAPES[shape]
|
|
versions = tmp_path / 'migrations' / 'versions'
|
|
versions.mkdir(parents=True)
|
|
gate = _gate_tree(tmp_path)
|
|
|
|
planted = versions / 'zz01_planted.py'
|
|
planted.write_text(_migration_module(body), encoding='utf-8')
|
|
dirty = subprocess.run(['bash', str(gate)], cwd=str(tmp_path),
|
|
capture_output=True, text=True)
|
|
assert dirty.returncode == 1, (
|
|
'the gate passed a {0} column:\n{1}'.format(shape, dirty.stdout))
|
|
assert expected in dirty.stdout, (
|
|
'the gate failed but never named {0}:\n{1}'.format(
|
|
expected, dirty.stdout))
|
|
|
|
|
|
@pytest.mark.skipif(shutil.which('bash') is None, reason='needs bash')
|
|
def test_the_gate_passes_well_named_columns(tmp_path):
|
|
"""The other half: a gate that fails on everything is no better.
|
|
|
|
Includes a drop_column with a bad name, which is out of scope on purpose.
|
|
"""
|
|
versions = tmp_path / 'migrations' / 'versions'
|
|
versions.mkdir(parents=True)
|
|
gate = _gate_tree(tmp_path)
|
|
(versions / 'zz01_clean.py').write_text(
|
|
_migration_module(PLANTED_CLEAN_BODY), encoding='utf-8')
|
|
clean = subprocess.run(['bash', str(gate)], cwd=str(tmp_path),
|
|
capture_output=True, text=True)
|
|
assert clean.returncode == 0, (
|
|
'the gate failed on lowercase concatenated names:\n' + clean.stdout)
|
|
|
|
|
|
@pytest.mark.skipif(shutil.which('bash') is None, reason='needs bash')
|
|
def test_the_site_literal_scan_honours_its_include_list(tmp_path):
|
|
"""The ADR-015 scan must read the file types it declares, and no others.
|
|
|
|
The scan carried --exclude=README.md AHEAD of its --include list, and on GNU
|
|
grep a file-level exclude in that position cancels every include on the
|
|
command. So the scan was reading EVERY file under agent/ and the other
|
|
scanned trees, and --include='*.cs' was decorative - adding agent/ did not
|
|
mean the C# agent was covered for the reason the flags claimed, and any
|
|
later narrowing of the include list would have been silently inert.
|
|
|
|
The .cs half is covered by the test above. This is the other half: a file
|
|
type the scan does not declare must not fail it, which is only true when the
|
|
include list is actually in force. Before the fix this run came back red on
|
|
notes.txt.
|
|
"""
|
|
gate = _gate_tree(tmp_path)
|
|
agent = tmp_path / 'agent' / 'Probe'
|
|
agent.mkdir(parents=True)
|
|
for name in ('notes.txt', 'Probe.csproj', 'README.md'):
|
|
(agent / name).write_text(
|
|
'shipped to ' + PLANTED_SITE_HOST + '\n', encoding='utf-8')
|
|
|
|
result = subprocess.run(['bash', str(gate)], cwd=str(tmp_path),
|
|
capture_output=True, text=True)
|
|
assert result.returncode == 0, (
|
|
'the scan read a file type it does not declare:\n' + result.stdout)
|