Skip to content

Commit 92e20da

Browse files
youknowdotyouknowdot
authored
Fix update_lib marker preservation for inline classes (#9131)
* Preserve inherited test markers in inline class migrations Expand canonical inline class suites before appending retained inherited overrides, and validate generated syntax. Run focused migration-tool tests in a path-filtered Python-only CI job. Assisted-by: OpenAI Codex:model version unavailable * Run migration tool tests through existing CI conditions Keep Rust build gates unchanged and run focused Python migration tests on Linux for tool-only changes. Remove the separate workflow. Assisted-by: OpenAI Codex:model version unavailable
1 parent 79e7c9d commit 92e20da

3 files changed

Lines changed: 90 additions & 5 deletions

File tree

‎.github/workflows/ci.yaml‎

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ jobs:
4343
outputs:
4444
# Flag that is raised when any rust code is changed.
4545
rust_code: ${{ steps.check_rust_code.outputs.changed }}
46+
update_lib: ${{ steps.check_update_lib.outputs.changed }}
4647
# WASM demo / notebook / example_projects, independent of rust_code.
4748
wasm_code: ${{ steps.check_wasm_code.outputs.changed }}
4849
# Cross-target cargo check is skipped unless the diff can change
@@ -87,6 +88,17 @@ jobs:
8788
env:
8889
MERGE_BASE: ${{ steps.merge_base.outputs.sha }}
8990

91+
- name: Check if update_lib changed
92+
id: check_update_lib
93+
run: |
94+
if git diff --quiet "${MERGE_BASE}...HEAD" -- ':scripts/update_lib/**' ':.python-version'; then
95+
echo "changed=false" >> "$GITHUB_OUTPUT"
96+
else
97+
echo "changed=true" >> "$GITHUB_OUTPUT"
98+
fi
99+
env:
100+
MERGE_BASE: ${{ steps.merge_base.outputs.sha }}
101+
90102
- name: Check if WASM package or demo sources changed
91103
id: check_wasm_code
92104
run: |
@@ -209,6 +221,7 @@ jobs:
209221
env:
210222
RUST_BACKTRACE: full
211223
RUN_RUST_TESTS: ${{ needs.determine_changes.outputs.rust_code == 'true' || github.ref == 'refs/heads/main' }}
224+
RUN_UPDATE_LIB_TESTS: ${{ needs.determine_changes.outputs.update_lib == 'true' }}
212225
# Named after the matrix entry rather than left to be named for it: a
213226
# generated name lists every value in the entry, so adding or removing one
214227
# renames the check and drops it from the required list.
@@ -222,10 +235,10 @@ jobs:
222235
steps:
223236
- name: Skip rust tests when sources did not change
224237
if: env.RUN_RUST_TESTS != 'true'
225-
run: echo "No rust source changes; required check is green without rebuilding."
238+
run: echo "No rust source changes; skipping Rust build and tests."
226239

227240
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
228-
if: env.RUN_RUST_TESTS == 'true'
241+
if: env.RUN_RUST_TESTS == 'true' || (env.RUN_UPDATE_LIB_TESTS == 'true' && runner.os == 'Linux')
229242
with:
230243
persist-credentials: false
231244

@@ -345,9 +358,19 @@ jobs:
345358
if: env.RUN_RUST_TESTS == 'true' && runner.os == 'Linux'
346359
run: cargo build --locked
347360

361+
- uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0
362+
if: env.RUN_RUST_TESTS != 'true' && env.RUN_UPDATE_LIB_TESTS == 'true' && runner.os == 'Linux'
363+
with:
364+
python-version-file: .python-version
365+
348366
- name: run update_lib tests
349-
if: env.RUN_RUST_TESTS == 'true' && runner.os == 'Linux'
350-
run: target/debug/rustpython -m unittest discover -s scripts/update_lib/tests -v
367+
if: (env.RUN_RUST_TESTS == 'true' || env.RUN_UPDATE_LIB_TESTS == 'true') && runner.os == 'Linux'
368+
run: |
369+
if [ "$RUN_RUST_TESTS" = "true" ]; then
370+
target/debug/rustpython -m unittest discover -s scripts/update_lib/tests -v
371+
else
372+
python -m unittest update_lib.tests.test_patch_spec update_lib.tests.test_migrate -v
373+
fi
351374
env:
352375
PYTHONPATH: scripts
353376

@@ -1090,7 +1113,7 @@ jobs:
10901113
steps:
10911114
- name: Skip wasm-wasi when rust sources did not change
10921115
if: env.RUN_WASM_WASI != 'true'
1093-
run: echo "No rust source changes; required check is green without rebuilding."
1116+
run: echo "No rust source changes; skipping Rust build and tests."
10941117

10951118
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
10961119
if: env.RUN_WASM_WASI == 'true'

‎scripts/update_lib/patch_spec.py‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -414,8 +414,36 @@ def _find_import_insert_line(tree: ast.Module) -> int:
414414
return 0
415415

416416

417+
def _expand_inline_class_suites(contents: str, patches: Patches) -> str:
418+
"""Make room for inherited overrides without changing existing class bodies."""
419+
tree = ast.parse(contents)
420+
lines = contents.splitlines()
421+
replacements = []
422+
for node in _iter_module_classes(tree):
423+
methods = {method.name for method in _iter_class_methods(node)}
424+
inherited = set(patches.get(node.name, {})) - methods - {"__self__"}
425+
if not inherited:
426+
continue
427+
first = node.body[0]
428+
line = lines[first.lineno - 1].encode("utf-8")
429+
header = line[: first.col_offset].decode("utf-8")
430+
if not header.strip():
431+
continue
432+
body = line[first.col_offset :].decode("utf-8")
433+
replacements.append(
434+
(
435+
first.lineno - 1,
436+
header.rstrip() + "\n" + " " * (node.col_offset + 4) + body,
437+
)
438+
)
439+
for index, replacement in sorted(replacements, reverse=True):
440+
lines[index] = replacement
441+
return "\n".join(lines) + "\n"
442+
443+
417444
def apply_patches(contents: str, patches: Patches) -> str:
418445
"""Apply patches to file contents and return modified contents."""
446+
contents = _expand_inline_class_suites(contents, patches)
419447
tree = ast.parse(contents)
420448
lines = contents.splitlines()
421449

@@ -436,6 +464,7 @@ def apply_patches(contents: str, patches: Patches) -> str:
436464
lines.insert(lineno, patch)
437465

438466
joined = "\n".join(lines)
467+
ast.parse(joined)
439468
return f"{joined}\n"
440469

441470

‎scripts/update_lib/tests/test_patch_spec.py‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,39 @@ def test_one(self):
374374
self.assertIn("@some_decorator", result)
375375
self.assertIn("@unittest.expectedFailure", result)
376376

377+
def test_inherited_override_expands_inline_class(self):
378+
for source in (
379+
"class Child(Base): pass # keep comment\n",
380+
"class Child(Base): value = 1; other = 2\n",
381+
"class Child(\n Base\n): pass\n",
382+
"if True:\n class Child(Base): pass\n",
383+
"class Café(Base): pass\n",
384+
):
385+
with self.subTest(source=source):
386+
name = "Café" if "Café" in source else "Child"
387+
patches = {
388+
name: {
389+
"test_parent": [PatchSpec(UtMethod.ExpectedFailure, None, "")]
390+
}
391+
}
392+
before = ast.parse(source)
393+
result = apply_patches(source, patches)
394+
after = ast.parse(result)
395+
old_class = next(
396+
node for node in ast.walk(before) if isinstance(node, ast.ClassDef)
397+
)
398+
new_class = next(
399+
node for node in ast.walk(after) if isinstance(node, ast.ClassDef)
400+
)
401+
self.assertEqual(
402+
[ast.dump(node) for node in old_class.body],
403+
[ast.dump(node) for node in new_class.body[:-1]],
404+
)
405+
self.assertEqual(new_class.body[-1].name, "test_parent")
406+
self.assertIn("return super().test_parent()", result)
407+
if "# keep comment" in source:
408+
self.assertIn("# keep comment", result)
409+
377410
def test_apply_inherited_method(self):
378411
"""Test applying patch to inherited method (creates override)."""
379412
code = """import unittest

0 commit comments

Comments
 (0)