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

Update to hypothesis 4.4.3 #1

Merged
merged 10 commits into from
Feb 10, 2019
Merged

Conversation

vxgmichel
Copy link
Contributor

... and bump version 0.3.0.

@codecov
Copy link

codecov bot commented Jan 31, 2019

Codecov Report

Merging #1 into master will increase coverage by 6.13%.
The diff coverage is 92.66%.

@@            Coverage Diff             @@
##           master       #1      +/-   ##
==========================================
+ Coverage   89.69%   95.83%   +6.13%     
==========================================
  Files           5        5              
  Lines         262      288      +26     
  Branches       35       33       -2     
==========================================
+ Hits          235      276      +41     
+ Misses         16        8       -8     
+ Partials       11        4       -7
Impacted Files Coverage Δ
hypothesis_trio/_version.py 100% <100%> (ø) ⬆️
hypothesis_trio/stateful.py 90.51% <90%> (+18.06%) ⬆️
hypothesis_trio/_tests/test_async_stateful.py 99.29% <97.36%> (-0.71%) ⬇️

@vxgmichel vxgmichel force-pushed the hypothesis-443 branch 3 times, most recently from bfa3300 to 159dab2 Compare January 31, 2019 17:07
.travis.yml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@Zac-HD
Copy link
Member

Zac-HD commented Feb 1, 2019

The problem here is that hypothesis_trio/stateful.py is mostly a copy-paste from functions and classes from hypothesis/stateful.py with a bit of extra trio stuff. So there's a very good chance that it's not compatible with previous or future versions of hypothesis. I can check that in the file history.
A better approach would probably be to add support for async state machines to hypothesis. I guess this should be discussed with the maintainers of the project.

Hi all, Hypothesis maintainer here 😄
(and it's probably relevant that I've got Trio and Pytest commit bits too)

The idea of adding async state machines has come up before, e.g. in HypothesisWorks/hypothesis#1709, but we don't have the bandwidth or expertise to maintain a general-purpose or generic async stateful testing framework.

More importantly, every test case must be deterministic for Hypothesis to really work. That means task scheduling and IO timing/chunking need to be deterministic, and ideally Hypothesis would control the IO part too (see python-trio/trio#239 for more thoughts). hypothesis-trio doesn't manage this currently, though I've been working on the scheduler story for a while now and getting close. (without that, the call order of async methods is not reliably related to the execution order and Hypothesis can't do much)

I haven't heard reports of hypothesis-trio being broken though, so I guess it's mostly working in practice? Pinning hypothesis~=4.0 should be fine then; we take semver seriously. Note though that we intend to deprecate GenericStateMachine soon and remove it early next year - I'd go ahead and remove it here.

Suggestions:

  • Pin hypothesis~=4.0
  • Accept that hypothesis-trio is already broken, and the parts to fix it don't exist yet
  • Don't spend much time working over a broken foundation for stateful testing
  • Create Hypothesis strategies for interactive IO
    (e.g. "this kind of message", "a channel from which <these messages> can be read", etc.)

@vxgmichel
Copy link
Contributor Author

vxgmichel commented Feb 1, 2019

Hi all, Hypothesis maintainer here 😄

Hi @Zac-HD, thank you for your work :)

I haven't heard reports of hypothesis-trio being broken though, so I guess it's mostly working in practice?

Well, this commit got rid of StateMachineRunner so the current version of hypothesis-trio is definitely broken for hypothesis >= 3.84.0. It does work for the project I'm currently working on though, which pins the hypothesis version to 3.82.2. I don't know whether or not hypothesis-trio works when hypothesis is in between those two bounds.

Pinning hypothesis~=4.0 should be fine then; we take semver seriously.

Well, the problem here is that I had to copy-paste the full run_state_machine_as_test function (I suspect it's also what has been done before), and that hypothesis-trio is patching the hypothesis.stateful module.

So let's say you fix some bug or behavior in this function and release hypothesis 4.4.4. Then consider a user with an environment including hypothesis-4.4.4 and hypothesis-trio-0.3.0. Depending on whether the user imports hypothesis_trio he might or might not get the fix, which I think is very confusing.

I guess we could get rid of the monkey-patching code, but it would still be weird for a user to not benefit for a fix that has been advertised in the hypothesis changelog. That's why I thought strict pinning was the lesser of two evils, until someone tackles the HypothesisWorks/hypothesis#1709 issue.

Note though that we intend to deprecate GenericStateMachine soon and remove it early next year - I'd go ahead and remove it here.

[edit] Ok I can do that! Done!

@Zac-HD
Copy link
Member

Zac-HD commented Feb 2, 2019

I think we can get a decent compromise:

def run_state_machine_as_test_patched(state_machine_factory, settings=None):
    if issubclass(state_machine_factory, TrioRuleBasedStateMachine):
        run_trio_state_machine_as_test(state_machine_factory, settings=settings)
    original_run_state_machine_as_test(state_machine_factory, settings=settings)

# And to replace the current monkeypatching...
original_run_state_machine_as_test = hypothesis.stateful.run_state_machine_as_test
hypothesis.stateful.run_state_machine_as_test = run_state_machine_as_test_patched

This allows the latest Hypothesis features to be used for everything except TrioRuleBasedStateMachines, which will be pinned to whatever they got copy-pasted out of.

@vxgmichel
Copy link
Contributor Author

vxgmichel commented Feb 2, 2019

I think we can get a decent compromise

Alright I'll change that then, and add hypothesis~=4.0 to the dependencies.

@vxgmichel
Copy link
Contributor Author

vxgmichel commented Feb 2, 2019

@Zac-HD I just realized your suggestion breaks the following use case:

my_machine = MyTrioStateMachine(*some_custom_args)
run_state_machine_as_test(lambda: my_machine)

Maybe a better solution is simply to remove the monkey patching and let user choose between hypothesis.stateful.run_state_machine_as_test and hypothesis_trio.stateful.run_state_machine_as_test.

Or make it even more explicit by getting rid of the state machine detection and exposing run_trio_state_machine_as_test. I honestly don't know what's best, what do you think?

@Zac-HD
Copy link
Member

Zac-HD commented Feb 3, 2019

Does that use-case come up often? Calling run_state_machine_as_test directly also breaks Hypothesis' statistics reporting 😕

I think we need the monkeypatching to make the .TestCase attribute work, or else override that for the TrioRuleBasedStateMachine class. Personally I'd monkeypatch it and expose run_trio_state_machine as part of hypothesis-trio's public API so both work.

@vxgmichel
Copy link
Contributor Author

@Zac-HD

Calling run_state_machine_as_test directly also breaks Hypothesis' statistics reporting

Oh I didn't know about that.

Personally I'd monkeypatch it and expose run_trio_state_machine as part of hypothesis-trio's public API so both work.

Alright, I just pushed 84948d8 if that's okay with you.

@vxgmichel vxgmichel force-pushed the hypothesis-443 branch 5 times, most recently from 12154a9 to 85e5898 Compare February 8, 2019 15:36
@vxgmichel vxgmichel force-pushed the hypothesis-443 branch 4 times, most recently from 9ca3a9a to 87db684 Compare February 8, 2019 16:41
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is a great improvement on the status quo, a fantastic first contributionm, and a solid 0.3 release. Thanks so much Vincent!

@Zac-HD Zac-HD merged commit 5c020ed into python-trio:master Feb 10, 2019
@Zac-HD
Copy link
Member

Zac-HD commented Feb 10, 2019

@njsmith can you cut a release for us? Thanks 😄

@touilleMan
Copy link
Member

@vxgmichel @Zac-HD version 0.3.0 has been released \o/

@Zac-HD
Copy link
Member

Zac-HD commented Feb 11, 2019

Thanks to all involved!

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

Successfully merging this pull request may close these issues.

4 participants