Refactor pytests to make pickle testing in-memory #924
Conversation
There was a problem hiding this comment.
Looks like this pickle file hasn't been removed. It needs to be removed and probably added to tests/.gitignore.
There's more files missed like this.
There was a problem hiding this comment.
So I think the file was removed the first time around. But I think Github shows the diffs for binary files that don't end with the extension .0 as a bit different for the other files. I tried adding them back, checked that there were no diffs and then deleted them again using git rm and I do see the files being shown in the delete mode
delete mode 100644 tests/multidict-pure-python.pickle.1
delete mode 100644 tests/multidict-pure-python.pickle.2
delete mode 100644 tests/multidict-pure-python.pickle.3
delete mode 100644 tests/multidict-pure-python.pickle.4
delete mode 100644 tests/multidict-pure-python.pickle.5
But the UI shows the same. Let me know if you have any other ideas on how to address this
There was a problem hiding this comment.
@vrdn-23 your console snippet shows a multidict file, not cimultidict.
There was a problem hiding this comment.
Try git rm -f tests/*.pickle.*.
There was a problem hiding this comment.
I guess, it might be GitHub's poor data representation, after all (although, I haven't checked in detail).
Anyway, per https://github.com/aio-libs/multidict/pull/924/files#r1463633143, we might need to keep the files, after all.
There was a problem hiding this comment.
Okay. I'm following the different threads but I'm not sure what the final resolution is. What is the final specification/requirements we want from this change? Could you update #922 with the new changes/ set of requirements that we need?
There was a problem hiding this comment.
You're right. I'll try to circle back later.
| @@ -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))) | |||
There was a problem hiding this comment.
Does this work without turning the generator into a list?
There was a problem hiding this comment.
If not, please use tuple instead.
There was a problem hiding this comment.
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?
| ) -> bytes: | ||
| """Generate an in-memory pickle of the multi-dict object.""" | ||
| proto = request.param | ||
| d = any_multidict_class([("a", 1), ("a", 2)]) |
There was a problem hiding this comment.
Please, use a reasonable variable name for the data you're storing there.
| @@ -86,3 +86,6 @@ TAGS | |||
|
|
|||
| # pyenv | |||
| .python-version | |||
|
|
|||
| # .pkl files in tests folder | |||
| tests/*.pickle.* | |||
There was a problem hiding this comment.
Could you move this into tests/.gitignore?
Also, having text files without a trailing LF is a bad tone.
| multidict_implementation.tag, | ||
| ) | ||
| ) | ||
| def test_load_from_file( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
What do these changes do?
Refactors the pickling tests so that the pickles are stored in-memory and not on disk and move them into a purest function-scoped fixture.
Are there changes in behavior for the user?
No behavior changes for end user.
Related issue number
Fixes #922
Checklist