multidict icon indicating copy to clipboard operation
multidict copied to clipboard

[TODO] Replace `tests/gen_pickles.py` with a pytest plugin

Open webknjaz opened this issue 1 year ago • 9 comments
trafficstars

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

webknjaz avatar Jan 16 '24 20:01 webknjaz

I've raised a PR here (https://github.com/aio-libs/multidict/pull/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!

vrdn-23 avatar Jan 17 '24 04:01 vrdn-23

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

webknjaz avatar Jan 17 '24 11:01 webknjaz

Sounds good. Let me know!

vrdn-23 avatar Jan 17 '24 17:01 vrdn-23

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.

Jamim avatar Jan 19 '24 01:01 Jamim

Feel free to submit an alternative PR.

webknjaz avatar Jan 20 '24 04:01 webknjaz