-
-
Notifications
You must be signed in to change notification settings - Fork 101
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
base: master
Are you sure you want to change the base?
Changes from 7 commits
0fbcb9a
e35a2ba
28d6acb
7b9f815
5300c00
ec3739b
3eba40e
5711740
187f1b1
37b0c71
b5c5d06
95b8064
a10412c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
webknjaz marked this conversation as resolved.
Show resolved
Hide resolved
|
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +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): | ||
def test_load_from_file( | ||
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. This test might need renaming since it no longer uses a file on disk. 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. 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. 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_classes, | ||
pickle_protocol, | ||
): | ||
multidict_class_name = any_multidict_class.__name__ | ||
pickle_file_basename = "-".join( | ||
( | ||
|
@@ -31,8 +36,6 @@ def test_load_from_file(any_multidict_class, multidict_implementation, pickle_pr | |
) | ||
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_classes[fname]) | ||
assert d == obj | ||
assert isinstance(obj, any_multidict_class) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usinggit rm
and I do see the files being shown in the delete modeBut 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vrdn-23 your console snippet shows a
multidict
file, notcimultidict
.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.
Try
git rm -f tests/*.pickle.*
.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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I'll try to circle back later.