Added hypothesis based tesing to SyncedCollection #373
Conversation
vishav1771
requested review from
bdice,
cbkerr and
a team
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## feature/synced_collections #373 +/- ##
===========================================================
Coverage 77.28% 77.28%
===========================================================
Files 47 47
Lines 7453 7453
===========================================================
Hits 5760 5760
Misses 1693 1693 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hi @vishav1771, I have review requests attached. Overall, it's important to be sure that test refactorings do not change the behavior of tests. Please take a look at my comments. I recommend that other reviewers wait to review this thoroughly until this first round of comments are handled.
| @@ -11,3 +11,4 @@ pytest-subtests==0.3.1 | |||
| pydocstyle==5.0.2 | |||
| pytest-cov==2.10.0 | |||
| pymongo==3.10.1 | |||
| hypothesis==5.23.7 | |||
There was a problem hiding this comment.
Just so you know, hypothesis changes its version numbers a lot. Every PR that is merged causes a version update.
There was a problem hiding this comment.
I don't think that any action needs to be taken -- just letting you know.
| @@ -22,12 +26,20 @@ | |||
| except ImportError: | |||
| NUMPY = False | |||
|
|
|||
| # changing default deadline of hypothesis | |||
| settings.register_profile('ci', deadline=600) | |||
There was a problem hiding this comment.
Is this configurable via setup.cfg or another file? If so, I would prefer that.
| FN_JSON = 'test.json' | ||
|
|
||
| PRINTABLE_NO_DOTS = printable.replace('.', ' ') | ||
|
|
||
| JSONDataStrategy = st.recursive( |
There was a problem hiding this comment.
It may be desirable to put all of our strategies into a separate module like strategies.py if they are re-used in multiple test files.
| JSONDataStrategy = st.recursive( | ||
| st.none() | st.booleans() | st.floats(allow_nan=False) | st.text(printable), | ||
| lambda children: st.lists(children, max_size=5) | st.dictionaries( | ||
| st.text(PRINTABLE_NO_DOTS), children, max_size=5), max_leaves=5) |
There was a problem hiding this comment.
Define this after DictKeyStrategy and replace:
| st.text(PRINTABLE_NO_DOTS), children, max_size=5), max_leaves=5) | |
| DictKeyStrategy, children, max_size=5), max_leaves=5) |
tests/test_synced_collection.py
Outdated
| _backend_kwargs = {'filename': self._fn_, 'write_concern': False} | ||
| yield JSONDict(**_backend_kwargs) |
There was a problem hiding this comment.
Seems like this should just be defined in one line. We're not using any inheritance from the base class to the children classes so I don't see a reason to have the extra variable here.
| _backend_kwargs = {'filename': self._fn_, 'write_concern': False} | |
| yield JSONDict(**_backend_kwargs) | |
| yield JSONDict(filename=self._fn_, write_concern=False) |
| @@ -520,22 +546,30 @@ def test_update_recursive(self, synced_list): | |||
| self.store(data1) | |||
| assert synced_list == data1 | |||
|
|
|||
| # inavlid data in file | |||
| # invalid data in file | |||
| data2 = {'a': 1} | ||
| self.store(data2) | ||
| with pytest.raises(ValueError): | ||
| synced_list.load() | ||
|
|
||
| # reset the file |
There was a problem hiding this comment.
This may be another example of the same problematic pattern with pytest fixtures / hypothesis. I commented on this specifically because it doesn't look like the other instances and would be easy to miss in a refactoring.
| synced_list2.load() | ||
| assert len(synced_list2) == 1 | ||
| synced_list.clear() | ||
| synced_list2 = deepcopy(synced_list) # possibly unsafe |
There was a problem hiding this comment.
Why is this possibly unsafe? The previous code did not indicate that it could be unsafe.
There was a problem hiding this comment.
This is in reference to the warning we added to JSONDict. I forgot to add this last time.
| assert synced_list2[0] == testdata | ||
| del synced_list2 # possibly unsafe |
There was a problem hiding this comment.
This deletes the copy instead of the original. This changes the behavior of the test!
| @given(st.lists(JSONDataStrategy, max_size=5), JSONDataStrategy) | ||
| def test_nested_list(self, synced_list, testdata_list, testdata): | ||
| synced_list.reset([testdata_list]) | ||
| child1 = synced_list[0] |
There was a problem hiding this comment.
This doesn't seem right. In the old code, child1 is a list: [2, 4]. Here, it looks like child1 is a "data element" (not a list). Carefully review this test.
|
I have no time to review this right now. But it seems to me that there are a lot of problems with hypothesis in combination with pytest and fixtures. I therefore recommend to abandon this refactoring effort. |
|
I agree with @csadorf and stopped just short of making the same recommendation (I was tired when I finished reviewing). @vishav1771 in terms of maximizing what you can accomplish with your last month of GSoC, I'd really like to see you work towards buffering, caching, and lazy loading. This testing PR could be picked up and finished at a later time. The expertise you've developed this summer in signac's internal data structures should be our first focus! 😊 |
|
I'm closing this PR for now. Will pick up it at later. |
Description
Added hypothesis based tesing to SyncedCollection
Motivation and Context
Related to #249
Types of Changes
1The change breaks (or has the potential to break) existing functionality.
Checklist:
If necessary:
Example for a changelog entry:
Fix issue with launching rockets to the moon (#101, #212).