-
Notifications
You must be signed in to change notification settings - Fork 392
refactor(tests): Use pytest collection to load JSON fixtures #1666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 12 commits
1bd56cb
d11c62a
0b6d57c
5941938
bb9c70c
5e9663e
226f22c
fac9433
b7f14c1
f955121
dde7532
0110511
9c028ec
ef89584
7697bf6
5799559
6a45550
09bb76c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,10 @@ | ||
| """Helpers to load tests from JSON files.""" | ||
|
|
||
| from .fixtures import ALL_FIXTURE_TYPES, Fixture, FixturesFile, FixtureTestItem | ||
| from .load_blockchain_tests import BlockchainTestFixture | ||
| from .load_state_tests import StateTestFixture | ||
|
|
||
| ALL_FIXTURE_TYPES.append(BlockchainTestFixture) | ||
| ALL_FIXTURE_TYPES.append(StateTestFixture) | ||
|
Comment on lines
+7
to
+8
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do these get executed when importing only, for example,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that's correct, it gets executed only when importing from
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh really? I thought parent modules were implicitly imported. I'm glad I checked! |
||
|
|
||
| __all__ = ["ALL_FIXTURE_TYPES", "Fixture", "FixturesFile", "FixtureTestItem"] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| """Base class for all fixture loaders.""" | ||
|
|
||
| import json | ||
| from abc import ABC, abstractmethod | ||
| from functools import cached_property | ||
| from typing import Any, Dict, Generator, List, Self, Type | ||
|
|
||
| from _pytest.nodes import Node | ||
| from pytest import Collector, File, Item | ||
|
|
||
|
|
||
| class FixtureTestItem(Item): | ||
| """ | ||
| Test item that comes from a fixture file. | ||
| """ | ||
|
|
||
| @property | ||
| def fixtures_file(self) -> "FixturesFile": | ||
| """Return the fixtures file from which the test was extracted.""" | ||
| raise NotImplementedError() | ||
|
|
||
|
|
||
| class Fixture(ABC): | ||
| """ | ||
| Single fixture from a JSON file. | ||
|
|
||
| It can be subclassed in combination with Item or Collector to create a | ||
| fixture that can be collected by pytest. | ||
| """ | ||
|
|
||
| test_file: str | ||
| test_key: str | ||
|
|
||
| def __init__( | ||
| self, | ||
| *args: Any, | ||
| test_file: str, | ||
| test_key: str, | ||
| **kwargs: Any, | ||
| ): | ||
| super().__init__(*args, **kwargs) | ||
| self.test_file = test_file | ||
| self.test_key = test_key | ||
|
|
||
| @classmethod | ||
| def from_parent( | ||
| cls, | ||
| parent: Node, | ||
| **kwargs: Any, | ||
| ) -> Self: | ||
| """Pytest hook that returns a fixture from a JSON file.""" | ||
| return super().from_parent( # type: ignore[misc] | ||
| parent=parent, **kwargs | ||
| ) | ||
|
|
||
| @classmethod | ||
| @abstractmethod | ||
| def is_format(cls, test_dict: Dict[str, Any]) -> bool: | ||
| """Return true if the object can be parsed as the fixture type.""" | ||
| pass | ||
|
|
||
|
|
||
| ALL_FIXTURE_TYPES: List[Type[Fixture]] = [] | ||
|
|
||
|
|
||
| class FixturesFile(File): | ||
| """Single JSON file containing fixtures.""" | ||
|
|
||
| @cached_property | ||
| def data(self) -> Dict[str, Any]: | ||
| """Return the JSON data of the full file.""" | ||
| # loaded once per worker per file (thanks to cached_property) | ||
| with self.fspath.open("r", encoding="utf-8") as f: | ||
| return json.load(f) | ||
|
|
||
| def clear_data_cache(self) -> None: | ||
| """Drop the data cache.""" | ||
| self.__dict__.pop("data", None) | ||
gurukamath marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| def collect( | ||
| self: Self, | ||
| ) -> Generator[Item | Collector, None, None]: | ||
| """Collect test cases from a single JSON fixtures file.""" | ||
| try: | ||
| loaded_file = self.data | ||
| except Exception: | ||
| return # Skip *.json files that are unreadable. | ||
| if isinstance(loaded_file, dict): | ||
| for key, test_dict in loaded_file.items(): | ||
| if not isinstance(test_dict, dict): | ||
| continue | ||
| for fixture_type in ALL_FIXTURE_TYPES: | ||
| if not fixture_type.is_format(test_dict): | ||
| continue | ||
| name = key | ||
| if "::" in name: | ||
| name = name.split("::")[1] | ||
| yield fixture_type.from_parent( # type: ignore | ||
| parent=self, | ||
| name=name, | ||
| test_file=str(self.path), | ||
| test_key=key, | ||
| ) | ||
| # Make sure we don't keep anything from collection in memory. | ||
| self.clear_data_cache() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels... strange? I can't quite put my finger on why.
Like, why do the fixtures contain python files at all? Is there another way we could accomplish the same thing (like excluding a directory)?
I dunno, this just triggers my spidey sense 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the culprit: https://github.com/ethereum/legacytests/tree/1f581b8ccdc4c63acf5f2c5c1b155c690c32a8eb/src/LegacyTests/Cancun/GeneralStateTestsFiller/Pyspecs
Checking out
ethereum/testsat this commit, when submodules are included, results in these python files being checked out too, and when collecting./tests/json_infra/fixturesfor JSON files, pytest tries to collect these files too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we exclude that directory on the command line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed that because with this approach the files are collected directly by pytest, as opposed to doing a glob in the test itself.