Skip to content

Commit 200ad59

Browse files
cosmo0920edsiper
authored andcommitted
github: scripts: commit_linter: Fix failing test cases
Signed-off-by: Hiroshi Hatake <[email protected]>
1 parent 2a990fd commit 200ad59

File tree

2 files changed

+71
-64
lines changed

2 files changed

+71
-64
lines changed

.github/scripts/commit_prefix_check.py

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,31 @@ def validate_commit(commit):
167167
expected_lower = {p.lower() for p in expected}
168168
subj_lower = subject_prefix.lower()
169169

170+
# ------------------------------------------------
171+
# Multiple-component detection
172+
# ------------------------------------------------
173+
# Treat pure build-related prefixes ("build:", "CMakeLists.txt:") as non-components.
174+
# Additionally, allow lib: to act as an umbrella for lib subcomponents
175+
# (e.g., ripser:, ripser_wrapper:) when subject prefix is lib:.
176+
non_build_prefixes = {
177+
p
178+
for p in expected_lower
179+
if p not in ("build:", "cmakelists.txt:")
180+
}
181+
182+
# Prefixes that are allowed to cover multiple subcomponents
183+
umbrella_prefixes = {"lib:"}
184+
185+
# If more than one non-build prefix is inferred AND the subject is not an umbrella
186+
# prefix, require split commits.
187+
if len(non_build_prefixes) > 1 and subj_lower not in umbrella_prefixes:
188+
expected_list = sorted(expected)
189+
expected_str = ", ".join(expected_list)
190+
return False, (
191+
f"Subject prefix '{subject_prefix}' does not match files changed.\n"
192+
f"Expected one of: {expected_str}"
193+
)
194+
170195
# Subject prefix must be one of the expected ones
171196
if subj_lower not in expected_lower:
172197
expected_list = sorted(expected)
@@ -177,8 +202,6 @@ def validate_commit(commit):
177202
)
178203

179204

180-
return False, f"Commit subject too long (>80 chars): '{first_line}'"
181-
182205
# If build is NOT optional and build: exists among expected,
183206
# then subject MUST be build:
184207
if not build_optional and "build:" in expected_lower and subj_lower != "build:":

.github/scripts/tests/test_commit_lint.py

Lines changed: 46 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -34,39 +34,37 @@ def test_infer_prefix_plugin():
3434
When a file is in plugins/<name>/, the prefix should be <name>:.
3535
This is the most common case for Fluent Bit commits modifying plugins.
3636
"""
37-
assert infer_prefix_from_paths(["plugins/out_s3/s3.c"]) == {"out_s3:"}
37+
prefixes, build_optional = infer_prefix_from_paths(["plugins/out_s3/s3.c"])
38+
assert prefixes == {"out_s3:"}
39+
assert build_optional is True
3840

3941
def test_infer_prefix_core_file():
4042
"""
4143
Test that core source files with flb_ prefix correctly infer the component name.
42-
43-
Files like src/flb_router.c should produce prefix "router:" by stripping
44-
the "flb_" prefix and file extension. This handles core library components.
4544
"""
46-
assert infer_prefix_from_paths(["src/flb_router.c"]) == {"router:"}
45+
prefixes, build_optional = infer_prefix_from_paths(["src/flb_router.c"])
46+
assert prefixes == {"router:"}
47+
assert build_optional is True
4748

4849
def test_infer_prefix_new_core_file():
4950
"""
5051
Test that core files with longer names and numbers are handled correctly.
51-
52-
Ensures the prefix inference works for files like flb_super_router2.c,
53-
extracting "super_router2:" correctly. This validates the regex handles
54-
underscores and numbers in component names.
5552
"""
56-
assert infer_prefix_from_paths(["src/flb_super_router2.c"]) == {"super_router2:"}
53+
prefixes, build_optional = infer_prefix_from_paths(["src/flb_super_router2.c"])
54+
assert prefixes == {"super_router2:"}
55+
assert build_optional is True
5756

5857
def test_infer_multiple_prefixes():
5958
"""
6059
Test that multiple files from different components produce multiple prefixes.
61-
62-
When a commit touches files from different components (e.g., a plugin and
63-
a core file), the inference should return all relevant prefixes. This helps
64-
detect commits that mix multiple subsystems, which should be split.
6560
"""
66-
assert infer_prefix_from_paths([
61+
prefixes, build_optional = infer_prefix_from_paths([
6762
"plugins/in_tail/tail.c",
6863
"src/flb_router.c"
69-
]) == {"in_tail:", "router:"}
64+
])
65+
assert prefixes == {"in_tail:", "router:"}
66+
# At least one real component touched → build is optional
67+
assert build_optional is True
7068

7169

7270
# -----------------------------------------------------------
@@ -251,20 +249,17 @@ def test_error_bad_squash_detected():
251249

252250
def test_error_multiple_prefixes_inferred_from_files():
253251
"""
254-
Test that commits touching multiple components are rejected.
255-
256-
When a commit modifies files from different components (e.g., both a plugin
257-
and core code), it should be split into separate commits. This keeps
258-
commits focused and makes reviews easier. The error message should list
259-
all expected prefixes.
252+
Commits touching multiple non-build components are rejected and must be
253+
split into separate commits, even if the subject matches one component.
260254
"""
261255
commit = make_commit(
262256
"in_tail: update handler\n\nSigned-off-by: User",
263257
["plugins/in_tail/tail.c", "src/flb_router.c"]
264258
)
265259
ok, msg = validate_commit(commit)
266260
assert ok is False
267-
assert "Expected one of:" in msg
261+
assert "does not match files changed" in msg
262+
268263

269264

270265
# -----------------------------------------------------------
@@ -295,77 +290,66 @@ def test_docs_or_ci_changes_allowed():
295290
def test_infer_prefix_empty_file_list():
296291
"""
297292
Test that an empty file list returns an empty prefix set.
298-
299-
Edge case: when no files are provided, the function should return
300-
an empty set rather than raising an error. This handles degenerate cases.
301293
"""
302-
assert infer_prefix_from_paths([]) == set()
294+
prefixes, build_optional = infer_prefix_from_paths([])
295+
assert prefixes == set()
296+
# No components, no CMakeLists → build not optional
297+
assert build_optional is False
303298

304299
def test_infer_prefix_src_subdirectory():
305300
"""
306301
Test prefix inference for files in src/ subdirectories.
307-
308-
Files in src/ subdirectories (like src/stream_processor/stream.c) that
309-
don't have the flb_ prefix should use the subdirectory name as the prefix.
310-
This handles organized core code that's not in the root src/ directory.
311302
"""
312-
assert infer_prefix_from_paths(["src/stream_processor/stream.c"]) == {"stream_processor:"}
303+
prefixes, build_optional = infer_prefix_from_paths(["src/stream_processor/stream.c"])
304+
assert prefixes == {"stream_processor:"}
305+
assert build_optional is True
313306

314307
def test_infer_prefix_unknown_paths():
315308
"""
316309
Test that files outside plugins/ and src/ don't generate prefixes.
317-
318-
Files in unknown locations (not plugins/ or src/) should not generate
319-
any prefix. This allows commits with only documentation, CI, or other
320-
non-code files to use generic prefixes.
321310
"""
322-
assert infer_prefix_from_paths(["random/file.c"]) == set()
311+
prefixes, build_optional = infer_prefix_from_paths(["random/file.c"])
312+
assert prefixes == set()
313+
assert build_optional is False
323314

324315
def test_infer_prefix_multiple_same_plugin():
325316
"""
326317
Test that multiple files from the same plugin yield a single prefix.
327-
328-
When a commit modifies multiple files within the same plugin directory
329-
(e.g., .c, .h, and config files), they should all produce the same prefix.
330-
This ensures commits modifying a plugin's internal structure are valid.
331318
"""
332-
assert infer_prefix_from_paths([
319+
prefixes, build_optional = infer_prefix_from_paths([
333320
"plugins/out_s3/s3.c",
334321
"plugins/out_s3/s3.h",
335322
"plugins/out_s3/config.c"
336-
]) == {"out_s3:"}
323+
])
324+
assert prefixes == {"out_s3:"}
325+
assert build_optional is True
337326

338327
def test_infer_prefix_plugin_with_underscores():
339328
"""
340329
Test that plugin names with underscores are handled correctly.
341-
342-
Plugin names can contain underscores (e.g., out_http). The prefix inference
343-
should preserve these underscores in the generated prefix.
344330
"""
345-
assert infer_prefix_from_paths(["plugins/out_http/http.c"]) == {"out_http:"}
331+
prefixes, build_optional = infer_prefix_from_paths(["plugins/out_http/http.c"])
332+
assert prefixes == {"out_http:"}
333+
assert build_optional is True
346334

347335
def test_infer_prefix_core_file_with_numbers():
348336
"""
349337
Test that core file names with numbers are handled correctly.
350-
351-
Core files like flb_http2.c should produce "http2:" (not "http2.c:").
352-
This validates that numbers in component names are preserved correctly.
353338
"""
354-
assert infer_prefix_from_paths(["src/flb_http2.c"]) == {"http2:"}
339+
prefixes, build_optional = infer_prefix_from_paths(["src/flb_http2.c"])
340+
assert prefixes == {"http2:"}
341+
assert build_optional is True
355342

356343
def test_infer_prefix_mixed_known_unknown():
357344
"""
358345
Test prefix inference with a mix of known and unknown file paths.
359-
360-
When a commit contains both files that generate prefixes (plugins/, src/)
361-
and files that don't (docs/, random files), only the known paths should
362-
contribute to the prefix set. Unknown paths are ignored.
363346
"""
364-
result = infer_prefix_from_paths([
347+
prefixes, build_optional = infer_prefix_from_paths([
365348
"plugins/in_tail/tail.c",
366349
"random/file.txt"
367350
])
368-
assert result == {"in_tail:"}
351+
assert prefixes == {"in_tail:"}
352+
assert build_optional is True
369353

370354

371355
# -----------------------------------------------------------
@@ -620,12 +604,12 @@ def test_valid_config_file_changes():
620604

621605
def test_error_multiple_prefixes_one_matches():
622606
"""
623-
Test that commits touching multiple components fail even if prefix matches one.
607+
When a commit touches multiple different components (e.g., a plugin and a
608+
core subsystem), the linter requires the commit to be split, even if the
609+
subject prefix matches one of those components.
624610
625-
When a commit modifies files from different components, it should be rejected
626-
even if the commit prefix matches one of the components. The error message
627-
should list all expected prefixes to help the developer split the commit.
628-
This enforces the principle of one logical change per commit.
611+
In this case, both 'in_tail:' and 'router:' are valid inferred prefixes,
612+
so the linter must reject the commit and report all expected prefixes.
629613
"""
630614
commit = make_commit(
631615
"in_tail: update\n\nSigned-off-by: User",

0 commit comments

Comments
 (0)