Skip to content

Commit cfb00e9

Browse files
authored
feat(fill): add coverage_missed_reason keyword argument to ported_from marker (#1746)
1 parent a23bf9b commit cfb00e9

File tree

5 files changed

+71
-7
lines changed

5 files changed

+71
-7
lines changed

.github/workflows/coverage.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ jobs:
121121
new_sources=$(echo "$CHANGED_TEST_FILES" | tr ',' '\n')
122122
echo "Changed or new test files: $new_sources"
123123
124-
uv run fill $new_sources --show-ported-from --clean --quiet --links-as-filled --ported-from-output-file ported_from_files.txt
124+
uv run fill $new_sources --show-ported-from --clean --quiet --links-as-filled --skip-coverage-missed-reason --ported-from-output-file ported_from_files.txt
125125
files=$(cat ported_from_files.txt)
126126
echo "Extracted converted tests: $files"
127127
if [[ -z "$files" ]]; then

CONTRIBUTING.md

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,21 +100,23 @@ We welcome contributions via pull requests! This section will guide you through
100100

101101
7. **For EVM Tests**: Review the cases in the [EIP checklist template](./docs/writing_tests/checklist_templates/eip_testing_checklist_template.md).
102102

103-
8. **Verify your changes** by running the appropriate checks:
103+
8. **For Porting Tests**: If you're porting tests from ethereum/tests, see the [porting guide](https://eest.ethereum.org/main/dev/porting_legacy_tests) for coverage analysis and using `--skip-coverage-missed-reason` when needed.
104+
105+
9. **Verify your changes** by running the appropriate checks:
104106

105107
```bash
106108
uvx --with=tox-uv tox -e lint,typecheck
107109
```
108110

109-
9. **Commit your changes** with meaningful commit messages (see [Commit Messages, Issues and PR Titles](#commit-messages-issue-and-pr-titles)).
111+
10. **Commit your changes** with meaningful commit messages (see [Commit Messages, Issues and PR Titles](#commit-messages-issue-and-pr-titles)).
110112

111-
10. **Push your branch** to your GitHub fork:
113+
11. **Push your branch** to your GitHub fork:
112114

113115
```bash
114116
git push -u origin your-branch-name
115117
```
116118

117-
10. **Create a pull request** by navigating to your fork on GitHub and clicking the "New Pull Request" button.
119+
12. **Create a pull request** by navigating to your fork on GitHub and clicking the "New Pull Request" button.
118120

119121
### Branch Naming Conventions
120122

@@ -277,6 +279,7 @@ We maintain high standards for our repository history to ensure it's clean, unde
277279
- The entry should clearly but concisely describe the change.
278280
- It must include a link to the PR in brackets (e.g., `([#1234](https://github.com/ethereum/execution-spec-tests/pull/1234))`).
279281
- Add any breaking changes at the top of the upcoming release section.
282+
- Changelog entries are automatically validated in CI to ensure proper formatting.
280283

281284
3. **Check PR title format**
282285

docs/dev/porting_legacy_tests.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,3 +88,22 @@ Follow the hyperlinks for lost base coverage (`LBC`) to address coverage gaps. H
8888
Also note that yul tests and possibly other tests used `CALLDATALOAD` that might no longer needed when designing a test with python. But we must always investigate if an opcode is not covered anymore to see if its okay.
8989

9090
For example, review the [discussion in this PR.](https://github.com/ethereum/execution-spec-tests/pull/975#issuecomment-2528792289)
91+
92+
## Resolving Coverage Gaps from Yul Compilation
93+
94+
When porting tests from ethereum/tests, you may encounter coverage gaps that are false positives. This commonly occurs because:
95+
96+
- **Original tests** often used Yul to define smart contracts, and solc compilation introduces additional opcodes that aren't specifically under test
97+
- **EEST ports** typically use the explicit EEST Opcode mini-language, which is more precise in opcode definition
98+
99+
If coverage analysis shows missing opcodes that were only present due to Yul compilation artifacts (not the actual feature being tested), this can be resolved during PR review by adding the `coverage_missed_reason` parameter:
100+
101+
```python
102+
@pytest.mark.ported_from(
103+
["path/to/original_test.json"],
104+
coverage_missed_reason="Missing opcodes are Yul compilation artifacts, not part of tested feature"
105+
)
106+
```
107+
108+
!!! note "Add coverage_missed_reason only after PR review"
109+
Only add `coverage_missed_reason` after PR review determines the coverage gap is acceptable, never preemptively. This helps maintain test coverage integrity while accounting for legitimate differences between Yul-based and EEST opcode-based implementations.

src/pytest_plugins/filler/ported_tests.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
2. Extract either the file paths (first positional argument) or PR URLs (pr keyword argument)
1818
3. Output a deduplicated, sorted list, one per line
1919
4. Skip test execution (collection only)
20+
5. Exclude tests with coverage_missed_reason from output
2021
2122
Marker Format:
2223
--------------
@@ -26,6 +27,7 @@
2627
"https://github.com/ethereum/execution-spec-tests/pull/1234",
2728
"https://github.com/ethereum/execution-spec-tests/pull/5678",
2829
],
30+
coverage_missed_reason="Optional reason for accepted coverage miss",
2931
)
3032
"""
3133

@@ -72,6 +74,19 @@ def pytest_addoption(parser: pytest.Parser):
7274
"Use '--show-ported-from=prs' to show PR URLs."
7375
),
7476
)
77+
ported_from_group.addoption(
78+
"--skip-coverage-missed-reason",
79+
action="store_true",
80+
dest="skip_coverage_missed_reason",
81+
default=False,
82+
help=(
83+
"When using --show-ported-from, exclude tests that have "
84+
"coverage_missed_reason in their @pytest.mark.ported_from marker. "
85+
"These are tests that were intentionally not ported from the original "
86+
"static filler files, typically because they are redundant or obsolete. "
87+
"This helps filter out accepted coverage gaps when analyzing test coverage."
88+
),
89+
)
7590
ported_from_group.addoption(
7691
"--ported-from-output-file",
7792
action="store",
@@ -107,6 +122,7 @@ def __init__(self, config) -> None:
107122
self.show_mode = config.getoption("show_ported_from")
108123
self.links_as_filled = config.getoption("links_as_filled")
109124
self.ported_from_output_file = config.getoption("ported_from_output_file")
125+
self.skip_coverage_missed_reason = config.getoption("skip_coverage_missed_reason")
110126

111127
@pytest.hookimpl(hookwrapper=True, trylast=True)
112128
def pytest_collection_modifyitems(
@@ -128,6 +144,13 @@ def pytest_collection_modifyitems(
128144
for item in items:
129145
ported_from_marker = item.get_closest_marker("ported_from")
130146
if ported_from_marker:
147+
# Skip tests with coverage_missed_reason
148+
if (
149+
"coverage_missed_reason" in ported_from_marker.kwargs
150+
and self.skip_coverage_missed_reason
151+
):
152+
continue
153+
131154
# Extract paths (first positional argument)
132155
if ported_from_marker.args:
133156
first_arg = ported_from_marker.args[0]

tests/frontier/identity_precompile/test_identity.py

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,56 +37,72 @@
3737
"https://github.com/ethereum/tests/blob/v17.1/src/GeneralStateTestsFiller/stPreCompiledContracts2/CallIdentity_4_gas18Filler.json",
3838
],
3939
pr=["https://github.com/ethereum/execution-spec-tests/pull/1344"],
40+
coverage_missed_reason="MPT related coverage lost, not relevant to this test",
4041
)
4142
@pytest.mark.valid_from("Byzantium")
4243
@pytest.mark.parametrize("call_type", [Op.CALL, Op.CALLCODE])
4344
@pytest.mark.parametrize(
4445
[
4546
"call_args",
4647
"memory_values",
48+
"contract_balance",
4749
"call_succeeds",
4850
],
4951
[
50-
pytest.param(CallArgs(gas=0xFF), (0x1,), True, id="identity_0"),
52+
pytest.param(CallArgs(gas=0xFF), (0x1,), 0x0, True, id="identity_0"),
5153
pytest.param(
5254
CallArgs(args_size=0x0),
5355
(0x0,),
56+
0x0,
5457
True,
5558
id="identity_1",
5659
),
60+
pytest.param(
61+
CallArgs(gas=0x30D40, value=0x1, args_size=0x0),
62+
(0x1,),
63+
0x1,
64+
True,
65+
id="identity_1_nonzerovalue",
66+
),
5767
pytest.param(
5868
CallArgs(gas=0x30D40, value=0x1, args_size=0x0),
5969
None,
70+
0x0,
6071
False,
61-
id="identity_1_nonzerovalue",
72+
id="identity_1_nonzerovalue_insufficient_balance",
6273
),
6374
pytest.param(
6475
CallArgs(args_size=0x25),
6576
(0xF34578907F,),
77+
0x0,
6678
True,
6779
id="identity_2",
6880
),
6981
pytest.param(
7082
CallArgs(args_size=0x25),
7183
(0xF34578907F,),
84+
0x0,
7285
True,
7386
id="identity_3",
7487
),
7588
pytest.param(
7689
CallArgs(gas=0x64),
7790
(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF,),
91+
0x0,
7892
True,
7993
id="identity_4",
8094
),
8195
pytest.param(
8296
CallArgs(gas=0x11),
8397
(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF,),
98+
0x0,
8499
False,
85100
id="identity_4_insufficient_gas",
86101
),
87102
pytest.param(
88103
CallArgs(gas=0x12),
89104
(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF,),
105+
0x0,
90106
True,
91107
id="identity_4_exact_gas",
92108
),
@@ -100,6 +116,7 @@ def test_call_identity_precompile(
100116
memory_values: Tuple[int, ...],
101117
call_succeeds: bool,
102118
tx_gas_limit: int,
119+
contract_balance: int,
103120
):
104121
"""Test identity precompile RETURNDATA is sized correctly based on the input size."""
105122
env = Environment()
@@ -116,6 +133,7 @@ def test_call_identity_precompile(
116133
account = pre.deploy_contract(
117134
contract_bytecode,
118135
storage=storage.canary(),
136+
balance=contract_balance,
119137
)
120138

121139
tx = Transaction(
@@ -135,6 +153,7 @@ def test_call_identity_precompile(
135153
"https://github.com/ethereum/tests/blob/v17.1/src/GeneralStateTestsFiller/stPreCompiledContracts2/CallIdentity_5Filler.json",
136154
],
137155
pr=["https://github.com/ethereum/execution-spec-tests/pull/1344"],
156+
coverage_missed_reason="MPT related coverage lost, not relevant to this test",
138157
)
139158
@pytest.mark.valid_from("Byzantium")
140159
@pytest.mark.parametrize("call_type", [Op.CALL, Op.CALLCODE])

0 commit comments

Comments
 (0)