Skip to content

Commit cf3d79f

Browse files
redsun82Copilot
andcommitted
Just: add a test suite for the tooling, and fix what it immediately caught
Nothing under `misc/just` had tests, and most of the ways it can break exit zero: an exclusion that stops excluding, a verb that stops being found, an argument that arrives split in two. Adds a plain-`unittest` suite, each case paired with a positive control so a passing test means the rule held rather than that nothing ran, reachable through the usual `test` recipe and run in CI on any change here. Writing it immediately caught real bugs: `just --dump` maps an alias name to an object rather than to its target, so resolving one handed a dict to a dict lookup and raised; fixed by reading the target where the plain recipe is looked up, and doing so once rather than once per use, so an alias landing on a forwarder is recognised as that forwarder rather than reached under the wrong root recipe. The two shapes a consuming root can actually take -- inheriting a `_root_<verb>` or replacing it -- are pinned by a test, since neither had ever been exercised; the README's claim about which paired repository gets a shared root recipe for free is dropped along with it, since it is no longer true of the one repository it named and nothing here could have caught it going stale. The suite itself gets fixed along the way: fixtures are checked against a real `just --dump`, comparing only the fields the code reads so they stay valid as `just` gains more; it honours `JUST_EXECUTABLE` so it runs reliably under `bazel test`'s sandboxed `PATH`; a mis-scoped entry point had left two thirds of it never run at all; and a verb now fails outright when a candidate justfile cannot be parsed, instead of silently dropping it and still exiting zero. Also points the Python tooling workflow's own path filter at itself, rather than at a file that does not exist in this repository, so editing the tooling actually triggers its CI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 567f607 commit cf3d79f

10 files changed

Lines changed: 1379 additions & 59 deletions

‎.github/workflows/python-tooling.yml‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,10 @@ on:
55
paths:
66
- "misc/bazel/**"
77
- "misc/codegen/**"
8+
- "misc/just/**"
89
- "misc/scripts/models-as-data/*.py"
910
- "*.bazel*"
10-
- .github/workflows/codegen.yml
11+
- .github/workflows/python-tooling.yml
1112
- .pre-commit-config.yaml
1213
branches:
1314
- main
@@ -33,3 +34,7 @@ jobs:
3334
shell: bash
3435
run: |
3536
bazel test //misc/codegen/...
37+
- name: Run just tooling tests
38+
shell: bash
39+
run: |
40+
bazel test //misc/just/...

‎misc/just/BUILD.bazel‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
load("@rules_python//python:defs.bzl", "py_library", "py_test")
2+
3+
py_library(
4+
name = "tooling",
5+
srcs = [
6+
"codeql_test_run.py",
7+
"forward_command.py",
8+
"language_tests.py",
9+
"run_on_files.py",
10+
],
11+
imports = ["."],
12+
visibility = ["//visibility:public"],
13+
)
14+
15+
[
16+
py_test(
17+
name = src[:-len(".py")],
18+
size = "small",
19+
srcs = [src],
20+
deps = [":tooling"],
21+
)
22+
for src in glob(["test_*.py"])
23+
]
24+
25+
test_suite(
26+
name = "test",
27+
)

‎misc/just/README.md‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,15 @@ therefore reads its default `.` as the whole repository rather than the director
5757
caller is in, so one that does its own work needs `[no-cd]` itself.
5858

5959
Being a recipe like any other, a `_root_<verb>` is inherited by a justfile importing the
60-
one defining it, which is how the internal repository gets this one for free. It runs
61-
once either way, as the two spellings are the same recipe. A root that defines its own
62-
instead replaces it, and then both run, each over the files of the repository that
63-
defines it: bazel formatting asks bazel from the root of the checkout the files belong
64-
to, so that a repository formats its own files with its own pin.
60+
one defining it. It runs once either way, as the two spellings are the same recipe. A
61+
root that defines its own instead replaces it, and then both run, each over the files of
62+
the repository that defines it: bazel formatting asks bazel from the root of the checkout
63+
the files belong to, so that a repository formats its own files with its own pin.
64+
65+
Nothing in a justfile says which of the two happened, so they are told apart by comparing
66+
the recipes. A root whose own copy is identical to the one it would otherwise inherit,
67+
down to the comment above it, is therefore taken for the inherited one and runs once.
68+
Copy such a recipe to start from if it helps, but leave its comment behind.
6569

6670
That last part is arranged by variables rather than by recipes. `set
6771
allow-duplicate-variables` in `defs.just` lets an importing justfile assign a variable
@@ -186,3 +190,17 @@ With no terminal to ask — a pipe, a log, a shell without `stty` — it falls b
186190
fixed 57 columns, so logs and CI output are the same width every time. That is one
187191
branch rather than a platform test: `just` runs `sh` everywhere, so Windows takes
188192
whichever arm fits rather than a path of its own.
193+
194+
# Tests
195+
196+
The scripts here have tests, run by `just test misc/just`, by `bazel test
197+
//misc/just/...`, or by CI on any change under this directory. They are plain
198+
`unittest`: this is the layer that runs other tooling, so it should not need a package
199+
manager to check itself.
200+
201+
They exist for the class of bug that leaves no trace. An exclusion that stops excluding,
202+
a verb that stops being found, an argument that arrives split in two — each of those
203+
still exits zero, and the only symptom is work quietly not done. So the tests come in
204+
pairs: one asserting the quiet outcome, and one positive control asserting the same
205+
setup can produce the loud one. Without the second, a passing first test is also what a
206+
test that runs nothing at all looks like.

‎misc/just/forward_command.py‎

Lines changed: 87 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -96,22 +96,31 @@ def list_value(assignments, name):
9696
return []
9797

9898

99+
def is_variadic(recipe):
100+
parameters = recipe["parameters"]
101+
return bool(parameters and parameters[-1]["kind"] in ("star", "plus"))
102+
103+
99104
def accepts(recipe, argc):
100105
"""Check whether a recipe can be called with a given number of arguments."""
101106
parameters = recipe["parameters"]
102-
variadic = parameters and parameters[-1]["kind"] in ("star", "plus")
103107
required = sum(
104108
1
105109
for parameter in parameters
106110
if parameter["default"] is None and parameter["kind"] != "star"
107111
)
108-
return required <= argc and (variadic or argc <= len(parameters))
112+
return required <= argc and (is_variadic(recipe) or argc <= len(parameters))
109113

110114

111115
def implements(dump, command, argc):
112116
"""Return the recipe a justfile runs for a command, if it has a usable one."""
113117
recipes = dump["recipes"]
114-
recipe = recipes.get(dump["aliases"].get(command, command))
118+
# An alias dumps as an object rather than as its target, so the name has to be read
119+
# out of it. Resolved once and used throughout: a verb reached by an alias is the
120+
# verb, so the justfile's own answer to it is named after the target too.
121+
alias = dump["aliases"].get(command)
122+
name = alias["target"] if alias else command
123+
recipe = recipes.get(name)
115124
if recipe is None or recipe["private"]:
116125
return None
117126
if any(
@@ -120,7 +129,7 @@ def implements(dump, command, argc):
120129
# Here the plain name is the forwarder's own, so it says nothing about what this
121130
# directory does. A justfile that both forwards and answers the command itself
122131
# spells its own answer `_root_<command>`, the one name the two can share.
123-
recipe = recipes.get(f"{ROOT_PREFIX}{command}")
132+
recipe = recipes.get(f"{ROOT_PREFIX}{name}")
124133
if recipe is None:
125134
return None
126135
return recipe if accepts(recipe, argc) else None
@@ -136,12 +145,14 @@ def dump_all(justfiles):
136145
with ThreadPoolExecutor(PROBE_WORKERS) as executor:
137146
dumps = list(executor.map(dump_justfile, justfiles))
138147
parsed = []
148+
failed = False
139149
for justfile, (dump, failure) in zip(justfiles, dumps):
140150
if dump is None:
151+
failed = True
141152
error(f"could not read {justfile}:\n{failure}")
142153
else:
143154
parsed.append((justfile, dump))
144-
return parsed
155+
return parsed, failed
145156

146157

147158
def git(directory, *args):
@@ -154,28 +165,26 @@ def git(directory, *args):
154165
)
155166
if result.returncode != 0:
156167
error(f"`git {' '.join(args)}` failed in {directory}:\n{result.stderr.strip()}")
157-
return []
158-
return result.stdout.splitlines()
168+
return [], True
169+
return result.stdout.splitlines(), False
159170

160171

161172
def submodules(directory):
162173
"""List the initialised submodules under a directory."""
163-
toplevel = git(directory, "rev-parse", "--show-toplevel")
174+
toplevel, failed = git(directory, "rev-parse", "--show-toplevel")
164175
if not toplevel or not (Path(toplevel[0]) / ".gitmodules").exists():
165-
return []
166-
paths = [
167-
Path(toplevel[0]) / line.split(" ", 1)[1]
168-
for line in git(
169-
toplevel[0], "config", "--file", ".gitmodules", "--get-regexp", r"\.path$"
170-
)
171-
]
176+
return [], failed
177+
lines, config_failed = git(
178+
toplevel[0], "config", "--file", ".gitmodules", "--get-regexp", r"\.path$"
179+
)
180+
paths = [Path(toplevel[0]) / line.split(" ", 1)[1] for line in lines]
172181
within = Path(directory).resolve()
173182
return [
174183
Path(directory) / os.path.relpath(path, within)
175184
for path in paths
176185
# An uninitialised submodule is an empty directory, with nothing to run.
177186
if path.is_relative_to(within) and (path / ".git").exists()
178-
]
187+
], failed or config_failed
179188

180189

181190
def find_justfiles(directory):
@@ -186,21 +195,21 @@ def find_justfiles(directory):
186195
worth finding.
187196
"""
188197
justfiles = set()
189-
for repository in [directory, *submodules(directory)]:
190-
justfiles.update(
191-
Path(repository) / line
192-
for line in git(
193-
repository,
194-
"ls-files",
195-
"--cached",
196-
"--others",
197-
"--exclude-standard",
198-
"--",
199-
"justfile",
200-
"*/justfile",
201-
)
198+
repositories, failed = submodules(directory)
199+
for repository in [directory, *repositories]:
200+
lines, git_failed = git(
201+
repository,
202+
"ls-files",
203+
"--cached",
204+
"--others",
205+
"--exclude-standard",
206+
"--",
207+
"justfile",
208+
"*/justfile",
202209
)
203-
return justfiles
210+
failed = failed or git_failed
211+
justfiles.update(Path(repository) / line for line in lines)
212+
return justfiles, failed
204213

205214

206215
def invocation_path(path, *, like):
@@ -225,7 +234,8 @@ def find_justfiles_above(command, arg):
225234
]
226235
found = []
227236
seen = []
228-
for justfile, dump in dump_all(candidates):
237+
parsed, failed = dump_all(candidates)
238+
for justfile, dump in parsed:
229239
# A justfile sitting exactly on the argument is called without it, as the
230240
# argument would only repeat where it already is.
231241
argc = 0 if justfile.parent.resolve() == directory else 1
@@ -236,13 +246,14 @@ def find_justfiles_above(command, arg):
236246
# Two repositories that each define a root recipe are not that case: the text
237247
# can match while the workspace, the tool it runs and the paths it excludes all
238248
# differ, so they have to stay apart. Nothing here says so. They are told apart
239-
# only by the doc comment one of them happens to carry, which means dropping
240-
# `doc` from this comparison silently discards an invocation unless a real
241-
# discriminator arrives in the same change.
249+
# only by whatever the two happened not to write identically, which today is a
250+
# doc comment on one of them, so dropping a field from this comparison silently
251+
# discards an invocation unless a real discriminator arrives in the same change.
252+
# `TestFindJustfilesAbove` holds both shapes.
242253
if recipe is not None and recipe not in seen:
243254
seen.append(recipe)
244255
found.append((justfile, recipe))
245-
return found
256+
return found, failed
246257

247258

248259
def find_justfiles_below(command, directory, covered=()):
@@ -257,10 +268,13 @@ def find_justfiles_below(command, directory, covered=()):
257268
but ask to be named rather than found.
258269
"""
259270
# The justfile at `directory` is covered by the search above it.
260-
candidates = sorted(find_justfiles(directory) - {Path(directory) / "justfile"})
271+
justfiles, failed = find_justfiles(directory)
272+
candidates = sorted(justfiles - {Path(directory) / "justfile"})
261273
matches = []
262274
opted_out = []
263-
for justfile, dump in dump_all(candidates):
275+
parsed, dump_failed = dump_all(candidates)
276+
failed = failed or dump_failed
277+
for justfile, dump in parsed:
264278
recipe = implements(dump, command, 0)
265279
if recipe is None:
266280
continue
@@ -277,7 +291,7 @@ def find_justfiles_below(command, directory, covered=()):
277291
continue
278292
contributed.setdefault(justfile.parent, []).append(recipe)
279293
found.append((justfile, recipe))
280-
return sorted(found, key=lambda match: match[0]), sorted(opted_out)
294+
return sorted(found, key=lambda match: match[0]), sorted(opted_out), failed
281295

282296

283297
def resolve(command, arg):
@@ -288,18 +302,18 @@ def resolve(command, arg):
288302
on. One found below gets its own directory instead, as there the argument only said
289303
where to look. Justfiles below that asked to be named are returned separately.
290304
"""
291-
above = find_justfiles_above(command, arg)
292-
resolved = [(justfile, arg, recipe["name"]) for justfile, recipe in above]
305+
above, failed = find_justfiles_above(command, arg)
306+
resolved = [(justfile, arg, recipe) for justfile, recipe in above]
293307
opted_out = []
294308
if os.path.isdir(arg):
295-
below, opted_out = find_justfiles_below(
309+
below, opted_out, below_failed = find_justfiles_below(
296310
command, arg, [recipe for _, recipe in above]
297311
)
312+
failed = failed or below_failed
298313
resolved += [
299-
(justfile, str(justfile.parent), recipe["name"])
300-
for justfile, recipe in below
314+
(justfile, str(justfile.parent), recipe) for justfile, recipe in below
301315
]
302-
return resolved, opted_out
316+
return resolved, opted_out, failed
303317

304318

305319
def report_opted_out(command, justfiles, *, ran):
@@ -334,6 +348,19 @@ def invoke_just(cwd, args):
334348
return 0
335349

336350

351+
def invocation_argument_groups(recipe, pos_args):
352+
"""Split arguments into what the recipe can be called with at once.
353+
354+
Every recipe reachable from a verb today is variadic or takes none, so this only
355+
ever yields one group. It is here because the arguments are paths and running the
356+
verb once per path is what a fixed-arity recipe would mean, where passing them
357+
together would fail on arity alone and say nothing useful about why.
358+
"""
359+
if is_variadic(recipe) or len(pos_args) <= 1:
360+
return [pos_args]
361+
return [[arg] for arg in pos_args]
362+
363+
337364
def forward(cmd, args):
338365
"""Forward a command to language-specific justfiles."""
339366
is_non_positional = re.compile(r"^(-.*|\+|[A-Z_][A-Z_0-9]*=.*)$")
@@ -342,27 +369,38 @@ def forward(cmd, args):
342369

343370
justfiles = {}
344371
opted_out = []
372+
resolution_failed = False
345373
for arg in positional_args or ["."]:
346-
resolved, skipped = resolve(cmd, arg)
374+
resolved, skipped, failed = resolve(cmd, arg)
347375
opted_out += skipped
376+
resolution_failed = resolution_failed or failed
348377
if not resolved:
378+
# A candidate that could not be read is reported below rather than here:
379+
# saying nothing matched would blame the argument for a broken justfile.
380+
if failed:
381+
continue
349382
error(f"No justfile found for {cmd} on {arg}")
350383
report_opted_out(cmd, skipped, ran=False)
351384
return 1
352385
for justfile, justfile_arg, recipe in resolved:
353386
justfiles.setdefault(justfile, (recipe, []))[1].append(justfile_arg)
354387

388+
if resolution_failed:
389+
report_opted_out(cmd, opted_out, ran=False)
390+
return 1
391+
355392
invocations = []
356393
for justfile, (recipe, pos_args) in justfiles.items():
357394
# An argument standing for the whole directory subsumes any more specific one
358395
# that ended up on the same justfile.
359396
whole_directory = str(justfile.parent)
360397
if whole_directory in pos_args:
361398
pos_args = [whole_directory]
362-
cwd, just_args = get_just_context(justfile, recipe, flags, pos_args)
363-
prefix = f"cd {cwd}; " if cwd else ""
364-
print(f"-> {prefix}just {' '.join(just_args)}")
365-
invocations.append((cwd, just_args))
399+
for group in invocation_argument_groups(recipe, pos_args):
400+
cwd, just_args = get_just_context(justfile, recipe["name"], flags, group)
401+
prefix = f"cd {cwd}; " if cwd else ""
402+
print(f"-> {prefix}just {' '.join(just_args)}")
403+
invocations.append((cwd, just_args))
366404

367405
report_opted_out(cmd, opted_out, ran=True)
368406

‎misc/just/justfile‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,15 @@
1+
import "defs.just"
2+
13
[no-cd]
24
[positional-arguments]
35
format *ARGS=".":
46
npx prettier --write "$@"
7+
8+
# Test the tooling in this directory.
9+
# Arguments are `unittest` ones, so a test file or a dotted test name rather than a
10+
# directory. A path given stays valid as this does not change directory: `PYTHONPATH` is
11+
# what lets the tests import the modules they are about, wherever they are run from.
12+
[no-cd]
13+
[positional-arguments]
14+
@test *ARGS=['discover', '-s', source_dir()]:
15+
PYTHONPATH="{{ source_dir() }}" {{ py }} -m unittest "$@"

0 commit comments

Comments
 (0)