Skip to content
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

Refactor pytests to make pickle testing in-memory #924

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
3 changes: 3 additions & 0 deletions .gitignore
Expand Up @@ -86,3 +86,6 @@ TAGS

# pyenv
.python-version

# .pkl files in tests folder
tests/*.pickle.*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this into tests/.gitignore?

Also, having text files without a trailing LF is a bad tone.

14 changes: 0 additions & 14 deletions tests/cimultidict-c-extension.pickle.0

This file was deleted.

Binary file removed tests/cimultidict-c-extension.pickle.1
Binary file not shown.
Binary file removed tests/cimultidict-c-extension.pickle.2
Binary file not shown.
Binary file removed tests/cimultidict-c-extension.pickle.3
Binary file not shown.
Binary file removed tests/cimultidict-c-extension.pickle.4
Binary file not shown.
Binary file removed tests/cimultidict-c-extension.pickle.5
Binary file not shown.
14 changes: 0 additions & 14 deletions tests/cimultidict-pure-python.pickle.0

This file was deleted.

Binary file removed tests/cimultidict-pure-python.pickle.1
Binary file not shown.
Binary file removed tests/cimultidict-pure-python.pickle.2
Binary file not shown.
Binary file removed tests/cimultidict-pure-python.pickle.3
Binary file not shown.
Binary file removed tests/cimultidict-pure-python.pickle.4
Binary file not shown.
Binary file removed tests/cimultidict-pure-python.pickle.5
Binary file not shown.
11 changes: 11 additions & 0 deletions tests/conftest.py
Expand Up @@ -158,6 +158,17 @@ def multidict_getversion_callable(multidict_module: ModuleType) -> Callable:
return multidict_module.getversion


@pytest.fixture(params=list(range(pickle.HIGHEST_PROTOCOL + 1)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work without turning the generator into a list?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, please use tuple instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already a pickle_protocol that is injected through pytest_generate_tests. Can't you depend on it instead of maintaining a copy of the same logic?

def in_memory_pickle_object(
request: pytest.FixtureRequest,
any_multidict_class: Type[MutableMultiMapping[str]],
) -> bytes:
"""Generate an in-memory pickle of the multi-dict object."""
proto = request.param
d = any_multidict_class([("a", 1), ("a", 2)])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, use a reasonable variable name for the data you're storing there.

return pickle.dumps(d, proto)


def pytest_addoption(
parser: pytest.Parser,
pluginmanager: pytest.PytestPluginManager,
Expand Down
28 changes: 0 additions & 28 deletions tests/gen_pickles.py

This file was deleted.

14 changes: 0 additions & 14 deletions tests/multidict-c-extension.pickle.0
webknjaz marked this conversation as resolved.
Show resolved Hide resolved

This file was deleted.

Binary file removed tests/multidict-c-extension.pickle.1
Binary file not shown.
Binary file removed tests/multidict-c-extension.pickle.2
Binary file not shown.
Binary file removed tests/multidict-c-extension.pickle.3
Binary file not shown.
Binary file removed tests/multidict-c-extension.pickle.4
Binary file not shown.
Binary file removed tests/multidict-c-extension.pickle.5
Binary file not shown.
14 changes: 0 additions & 14 deletions tests/multidict-pure-python.pickle.0

This file was deleted.

Binary file removed tests/multidict-pure-python.pickle.1
Binary file not shown.
Binary file removed tests/multidict-pure-python.pickle.2
Binary file not shown.
Binary file removed tests/multidict-pure-python.pickle.3
Binary file not shown.
Binary file removed tests/multidict-pure-python.pickle.4
Binary file not shown.
Binary file removed tests/multidict-pure-python.pickle.5
Binary file not shown.
18 changes: 6 additions & 12 deletions tests/test_pickle.py
Expand Up @@ -21,18 +21,12 @@ def test_pickle_proxy(any_multidict_class, any_multidict_proxy_class):
pickle.dumps(proxy)


def test_load_from_file(any_multidict_class, multidict_implementation, pickle_protocol):
multidict_class_name = any_multidict_class.__name__
pickle_file_basename = "-".join(
(
multidict_class_name.lower(),
multidict_implementation.tag,
)
)
def test_load_from_file(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test might need renaming since it no longer uses a file on disk.
Also, it seems like test_pickle might cover this case already.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTOH, upon further reflection, I think I might know why the actual files are committed in Git — this is to ensure that newer multidict versions can load pickles of the older ones.
So we might be treating this incorrectly.

We might need to have a CLI interface for making the files in pytest but keep them. A fixture w/o an in-memory strucutre would be useful, though.

any_multidict_class,
multidict_implementation,
in_memory_pickle_object,
):
d = any_multidict_class([("a", 1), ("a", 2)])
fname = f"{pickle_file_basename}.pickle.{pickle_protocol}"
p = here / fname
with p.open("rb") as f:
obj = pickle.load(f)
obj = pickle.loads(in_memory_pickle_object)
assert d == obj
assert isinstance(obj, any_multidict_class)