RMG-Py icon indicating copy to clipboard operation
RMG-Py copied to clipboard

Pickle testing not thorough enough

Open rwest opened this issue 3 years ago • 1 comments

Pull request #2308 fixed an issue where an object (in that case a GroupAtom) was not pickling all of its properties, which led to strange hard-to-debug behaviour. But it would be quite an easy thing to catch in a unit test. There was already a test_pickle(self) unit test that claims to "Test that a GroupAtom object can be successfully pickled and unpickled with no loss of information." but it clearly doesn't, quite, check everything. Probably an attribute was added after the unit test was made, and the test (and the __reduce__ method) weren't updated.

There may be other corners where pickling is not properly tested, and perhaps even some where it is broken.

Could/should we do some/all of:

  1. audit all pickle unit tests and check they're up to date and complete?

  2. devise a pickling unit test that is always up to date and complete and future-proof, and convert some/all of our pickling unit tests to use this mechanism?

  3. remind code authors and code reviewers to look out for unit tests, and to look out for __reduce__ functions

rwest avatar Jun 08 '22 20:06 rwest

Thanks for bringing this up. @hwpang and I can look into this unit test to figure out the issue and try to update it

kspieks avatar Jun 08 '22 21:06 kspieks

This issue is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant issue, otherwise it will automatically be closed in 30 days.

github-actions[bot] avatar Jun 21 '23 22:06 github-actions[bot]