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 pickle tests #938

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Jamim
Copy link
Contributor

@Jamim Jamim commented Feb 1, 2024

Hello @webknjaz,

What do these changes do?

The main goal of these changes is to make test_pickle.py more readable and eliminate get_pickles.py.

These changes:

  • add dict_data, pickled_data, and pickle_file_path fixtures
  • rename test_pickle to test_unpickle
  • add test_pickle_format_stability
  • replace test_load_from_file with test_pickle_backward_compatibility
  • replace get_pickles.py with test_write_pickle_file

Are there changes in behavior for the user?

No.

Related issue number

This PR is alternative to:

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

Best regards!

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Feb 1, 2024
@Jamim Jamim force-pushed the feature/refactor-pickle-tests branch from ea0e93e to 3eb82a4 Compare February 1, 2024 03:07
Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.63%. Comparing base (f705368) to head (f4b7524).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
+ Coverage   95.09%   95.63%   +0.53%     
==========================================
  Files          21       20       -1     
  Lines        2407     2404       -3     
  Branches      275      271       -4     
==========================================
+ Hits         2289     2299      +10     
+ Misses         46       33      -13     
  Partials       72       72              
Flag Coverage Δ
CI-GHA 95.63% <100.00%> (+0.53%) ⬆️
MyPy 67.35% <41.46%> (+0.29%) ⬆️
unit 99.51% <100.00%> (+0.58%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jamim Jamim force-pushed the feature/refactor-pickle-tests branch 4 times, most recently from e763ee5 to 4bb5852 Compare February 3, 2024 04:07
@Jamim Jamim force-pushed the feature/refactor-pickle-tests branch from 4bb5852 to f4b7524 Compare March 7, 2024 20:28
The main goal of these changes is to make test_pickle.py
more readable and eliminate get_pickles.py.

These changes:
  - add dict_data, pickled_data, and pickle_file_path fixtures
  - rename test_pickle to test_unpickle
  - add test_pickle_format_stability
  - replace test_load_from_file with test_pickle_backward_compatibility
  - replace get_pickles.py with test_write_pickle_file
@Jamim Jamim force-pushed the feature/refactor-pickle-tests branch from f4b7524 to b2b00a3 Compare March 19, 2024 21:26
@Jamim
Copy link
Contributor Author

Jamim commented Mar 19, 2024

Hello @webknjaz,

Would you mind reviewing this PR once you have a spare time?

Thanks in advance!

@webknjaz
Copy link
Member

webknjaz commented Sep 9, 2024

I wanted to reflect on what I prefer with this. #924 and this one made me realize that I'm not sure yet. So I'd keep it open for the time being.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants