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

[TODO] Migrate tests/gen_pickles.py into a pytest fixture #922

Open
6 tasks
webknjaz opened this issue Jan 16, 2024 · 5 comments · May be fixed by #924
Open
6 tasks

[TODO] Migrate tests/gen_pickles.py into a pytest fixture #922

webknjaz opened this issue Jan 16, 2024 · 5 comments · May be fixed by #924

Comments

@webknjaz
Copy link
Member

The idea is simple:

  • Make a session-scoped fixture in tests/conftest.py
  • Move the logic from gen_pickles.py there and generate the pickles in a tmppath-provided dir
  • Integrate that into the pickling tests
  • Delete tests/gen_pickles.py and the pickle files in the tests/ dir
  • Bonus points: keep pickles in-memory, so they don't hit the disk
  • Bonus 2: check if a session fixture is needed and switch to the normal function scope, if not
@vrdn-23
Copy link

vrdn-23 commented Jan 17, 2024

I've raised a PR here (#924). I think this addresses the ideas mentioned here. let me know if there's a better way to do this or if I'm barking up the wrong tree completely!
Thanks for all your help so far!

@webknjaz
Copy link
Member Author

I'll have a number of comments in the review there.

@vrdn-23
Copy link

vrdn-23 commented Jan 17, 2024

Sounds good. Let me know!

@Jamim
Copy link
Contributor

Jamim commented Jan 19, 2024

Hello @vrdn-23 and @webknjaz,

I tried to fix #924 locally and after a few iterations of a simplification I realized that the test_load_from_file test doesn't increase coverage since there's already the test_pickle test that does exactly the same, but in a more elegant and easy to understand way. If I didn't miss something, it's implicitly parametrized enough to cover all the cases. The only difference is that the initial implementation of test_load_from_file checks whether data pickled with some previous version of the multidict library still can be unpickled and matched with the current version data, but if you are going to eliminate the corresponding files, it seems reasonable to drop the test_load_from_file completely and rely on the test_pickle instead.

def test_pickle(any_multidict_class, pickle_protocol):
    d = any_multidict_class([("a", 1), ("a", 2)])
    pbytes = pickle.dumps(d, pickle_protocol)
    obj = pickle.loads(pbytes)
    assert d == obj
    assert isinstance(obj, any_multidict_class)

@webknjaz what do you think?

@vrdn-23, in case @webknjaz decide to keep this test, it would be better to use io.BytesIO in order to keep pickle.dumps/pickle.loads and not switch to pickle.dump/pickle.load, so the test_load_from_file would verify what its name suggests.

@webknjaz
Copy link
Member Author

Feel free to submit an alternative PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants