Skip to content

Commit 54a1241

Browse files
marioevzspencer-tb
andauthored
feat(github/cli): Fixture folder compare command (#1833)
* feat(cli): Add `compare_fixtures` command * feat(github): Use `compare_fixtures` during coverage * chore(clis): add exception handling for compare fixtures. * chore(docs): changelog. --------- Co-authored-by: spencer-tb <spencer@spencertaylorbrown.uk>
1 parent b4d7826 commit 54a1241

File tree

4 files changed

+181
-5
lines changed

4 files changed

+181
-5
lines changed

.github/scripts/fill_prepatched_tests.sh

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,10 @@ FILL_UNTIL="${5:-Cancun}"
1616
echo "--------------------"
1717
echo "converted-ethereum-tests.txt seem untouched, try to fill pre-patched version of .py files:"
1818

19+
PATCH_COMMIT=$(git rev-parse HEAD)
1920
git checkout main
20-
PREV_COMMIT=$(git rev-parse HEAD)
21-
echo "Checkout head $PREV_COMMIT"
21+
BASE_COMMIT=$(git rev-parse HEAD)
22+
echo "Checkout head $BASE_COMMIT"
2223

2324
echo "Select files that were changed and exist on the main branch:"
2425
echo "$MODIFIED_DELETED_FILES"
@@ -38,7 +39,19 @@ if grep -q "ERROR collecting test session" filloutput.log; then
3839
exit 1
3940
fi
4041

41-
# TODO: Here we can inspect $BASE_TEST_PATH vs $PATCH_TEST_PATH and remove fixtures with the same hash in both directories, to only leave fixtures that have been modified or removed,
42-
# and then set any_modified_fixtures=false if the fixture set before the PR is empty after this check.
43-
echo "any_modified_fixtures=true" >> "$GITHUB_OUTPUT"
42+
git checkout $PATCH_COMMIT
43+
echo "Checkout back to patch $PATCH_COMMIT"
44+
# abort-on-empty-patch is used to ensure that the patch folder is not empty after fixture removal.
45+
# If the patch folder would be empty, it means that fixtures were removed in the PR, in which case we still want to run the coverage check.
46+
uv run compare_fixtures --abort-on-empty-patch $BASE_TEST_PATH $PATCH_TEST_PATH
47+
48+
if [ -d $BASE_TEST_PATH ]; then
49+
# If the base folder is not empty, it means there's at least one fixture that was modified in the PR, continue with the coverage check.
50+
echo "Base folder is not empty after fixture comparison, continuing with coverage check."
51+
echo "any_modified_fixtures=true" >> "$GITHUB_OUTPUT"
52+
else
53+
# If the base folder is empty, it means there were no fixtures that were modified in the PR, or fixtures were only added, so we can skip the coverage check.
54+
echo "Base folder is empty after fixture comparison, skipping coverage check."
55+
echo "any_modified_fixtures=false" >> "$GITHUB_OUTPUT"
56+
fi
4457
exit 0

docs/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ Users can select any of the artifacts depending on their testing needs for their
7676
- 🔀 Add macOS hive development mode workaround to the docs [#1786](https://github.com/ethereum/execution-spec-tests/pull/1786).
7777
- 🔀 Refactor and clean up of exceptions including EOF exceptions within client specific mappers [#1803](https://github.com/ethereum/execution-spec-tests/pull/1803).
7878
- 🔀 Rename `tests/zkevm/` to `tests/benchmark/` and replace the `zkevm` pytest mark with `benchmark` [#1804](https://github.com/ethereum/execution-spec-tests/pull/1804).
79+
- 🔀 Add fixture comparison check to optimize coverage workflow in CI ([#1833](https://github.com/ethereum/execution-spec-tests/pull/1833)).
7980

8081
### 🧪 Test Cases
8182

pyproject.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ eest = "cli.eest.cli:eest"
105105
fillerconvert = "cli.fillerconvert.fillerconvert:main"
106106
groupstats = "cli.show_pre_alloc_group_stats:main"
107107
extract_config = "cli.extract_config:extract_config"
108+
compare_fixtures = "cli.compare_fixtures:main"
108109

109110
[tool.setuptools.packages.find]
110111
where = ["src"]

src/cli/compare_fixtures.py

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
"""
2+
Compare two fixture folders and remove duplicates based on fixture hashes.
3+
4+
This tool reads the .meta/index.json files from two fixture directories and identifies
5+
fixtures with identical hashes on a test case basis, then removes the duplicates from
6+
both of the folders. Used within the coverage workflow.
7+
"""
8+
9+
import json
10+
import shutil
11+
import sys
12+
from pathlib import Path
13+
from typing import Set
14+
15+
import click
16+
17+
from ethereum_test_base_types import HexNumber
18+
from ethereum_test_fixtures.consume import IndexFile, TestCaseIndexFile
19+
20+
21+
def get_index_path(folder: Path) -> Path:
22+
"""Get the path to an index in a given folder."""
23+
return folder / ".meta" / "index.json"
24+
25+
26+
def load_index(folder: Path) -> IndexFile:
27+
"""Load the index.json file from a fixture folder."""
28+
index_path = get_index_path(folder)
29+
if not index_path.exists():
30+
raise FileNotFoundError(f"Index file not found: {index_path}")
31+
32+
return IndexFile.model_validate_json(index_path.read_text())
33+
34+
35+
def get_fixture_hashes(index: IndexFile) -> Set[HexNumber]:
36+
"""Extract fixture hashes and their corresponding file paths from index."""
37+
hash_set = set()
38+
39+
for test_case in index.test_cases:
40+
if test_case.fixture_hash is None:
41+
continue
42+
hash_set.add(test_case.fixture_hash)
43+
44+
return hash_set
45+
46+
47+
def find_duplicates(base_hashes: Set[HexNumber], patch_hashes: Set[HexNumber]) -> Set[HexNumber]:
48+
"""Find fixture hashes that exist in both base and patch."""
49+
return base_hashes & patch_hashes
50+
51+
52+
def pop_by_hash(index: IndexFile, fixture_hash: HexNumber) -> TestCaseIndexFile:
53+
"""Pops a single test case from an index file by its hash."""
54+
for i in range(len(index.test_cases)):
55+
if index.test_cases[i].fixture_hash == fixture_hash:
56+
return index.test_cases.pop(i)
57+
raise Exception(f"Hash {fixture_hash} not found in index.")
58+
59+
60+
def remove_fixture_from_file(file: Path, test_case_id: str):
61+
"""Remove a single fixture by its ID from a generic fixture file."""
62+
try:
63+
# Load from json to a dict
64+
full_file = json.loads(file.read_text())
65+
full_file.pop(test_case_id)
66+
file.write_text(json.dumps(full_file, indent=2))
67+
except FileNotFoundError:
68+
raise FileNotFoundError(f"Fixture file not found: {file}") from None
69+
except KeyError:
70+
raise KeyError(f"Test case {test_case_id} not found in {file}") from None
71+
72+
73+
def remove_fixture(
74+
folder: Path,
75+
index: IndexFile,
76+
fixture_hash: HexNumber,
77+
dry_run: bool,
78+
):
79+
"""Remove a single fixture from a folder that matches the given hash."""
80+
test_case = pop_by_hash(index, fixture_hash)
81+
test_case_file = folder / test_case.json_path
82+
if dry_run:
83+
print(f"Remove {test_case.id} from {test_case_file}")
84+
else:
85+
remove_fixture_from_file(test_case_file, test_case.id)
86+
87+
88+
def rewrite_index(folder: Path, index: IndexFile, dry_run: bool):
89+
"""
90+
Rewrite the index to the correct index file, or if the test count was reduced to zero,
91+
the entire directory is deleted.
92+
"""
93+
if len(index.test_cases) > 0:
94+
# Just rewrite the index
95+
if not dry_run:
96+
with open(get_index_path(folder), "w") as f:
97+
f.write(index.model_dump_json(exclude_none=False, indent=2))
98+
else:
99+
print(f"Would rewrite index for {folder}")
100+
else:
101+
# Delete the folder
102+
if not dry_run:
103+
shutil.rmtree(folder)
104+
else:
105+
print(f"Would delete {folder}")
106+
107+
108+
@click.command()
109+
@click.argument("base", type=click.Path(exists=True, file_okay=False, path_type=Path))
110+
@click.argument("patch", type=click.Path(exists=True, file_okay=False, path_type=Path))
111+
@click.option(
112+
"--dry-run", is_flag=True, help="Show what would be removed without actually removing"
113+
)
114+
@click.option(
115+
"--abort-on-empty-patch",
116+
is_flag=True,
117+
help="Abort if the patch folder would be empty after fixture removal.",
118+
)
119+
def main(
120+
base: Path,
121+
patch: Path,
122+
dry_run: bool,
123+
abort_on_empty_patch: bool,
124+
):
125+
"""Compare two fixture folders and remove duplicates based on fixture hashes."""
126+
try:
127+
# Load indices
128+
base_index = load_index(base)
129+
base_hashes = get_fixture_hashes(base_index)
130+
131+
patch_index = load_index(patch)
132+
patch_hashes = get_fixture_hashes(patch_index)
133+
134+
# Find duplicates
135+
duplicate_hashes = find_duplicates(base_hashes, patch_hashes)
136+
137+
if not duplicate_hashes:
138+
click.echo("No duplicates found.")
139+
sys.exit(0)
140+
else:
141+
click.echo(f"Found {len(duplicate_hashes)} duplicates.")
142+
143+
if abort_on_empty_patch and duplicate_hashes == patch_hashes:
144+
click.echo("Patch folder would be empty after fixture removal.")
145+
sys.exit(0)
146+
147+
for duplicate_hash in duplicate_hashes:
148+
# Remove from both folders
149+
remove_fixture(base, base_index, duplicate_hash, dry_run)
150+
remove_fixture(patch, patch_index, duplicate_hash, dry_run)
151+
152+
# Rewrite indices if necessary
153+
rewrite_index(base, base_index, dry_run)
154+
rewrite_index(patch, patch_index, dry_run)
155+
except Exception as e:
156+
click.echo(f"Error: {e}", err=True)
157+
sys.exit(1)
158+
159+
160+
if __name__ == "__main__":
161+
main()

0 commit comments

Comments
 (0)