Hypothesis h2 settings equality #543

fredthomsen · 2017-04-10T02:45:46Z

WIP for equality and property test using hypothesis to address #466.

Lukasa

Cool, so this is a really good start! I'm going to ask @alexwlchan to do the majority of the reviewing here because he's the best placed to do it.

Lukasa · 2017-04-10T08:42:30Z

test/test_settings.py

@@ -333,7 +333,7 @@ def test_cannot_set_invalid_values_for_max_frame_size(self, val):
            assert e.value.error_code == h2.errors.ErrorCodes.PROTOCOL_ERROR
            assert s[h2.settings.SettingCodes.MAX_FRAME_SIZE] == 16384

-    @given(integers())
+    @given(strategies.integers())


What's the reasoning for this change?

I did this since I was using more functions within the strategies submodule along with integers. Probably just a nervous tick I have 😁 . I am happy to conform to whatever style you deem appropriate ie change this back and just import all the functions from the strategies submodule that I use.

I don't think it matters. 😉 Just curious.

It makes no difference, so I’d prefer to change it back to keep the diff focused. 🙂

Lukasa · 2017-04-10T08:44:44Z

test/test_settings.py

+                h2.settings.SettingCodes.MAX_HEADER_LIST_SIZE,
+            ]),
+            values=strategies.integers(min_value=0))
+        ) 


So this strategy generally seems ok, except that it'll frequently generate invalid initial settings that the settings object will reject. We'll need to expand this out to ensure that we only generate valid initial settings (or that we discard any attempt to create an invalid one).

Yup. Hadn't gotten there yet, but this is next.

Lukasa · 2017-04-10T08:45:43Z

test/test_settings.py

        assert not (a != b)

-    def test_different_eq(self):
+    @given(a=SettingsStrategy, b=SettingsStrategy)


So it's entirely possible that this would generate two settings objects that are identical. We need to be a bit careful here and construct a test that allows us to ensure that the two objects are different. In this case, the easiest way might be to create two strategies that cannot generate overlapping Settings objects in some way, or at least create a Settings object that our strategy cannot generate. Of course, if @alexwlchan has more useful things to suggest here that'd be really helpful (he's our resident Hypothesis expert).

Very true. I'll play around with this some more and see what advice @alexwlchan has.

I’m not sure Hypothesis has an easy way to generate two objects that are guaranteed to be distinct – any way we do that would rely on the equality operators, which are precisely what we’re trying to test!

Something like:

from hypothesis import assume @given(SettingsStrategy, SettingsStrategy) def test_equality_operators(x, y): assume(x != y): assert not (x == y)

Here assume will cause Hypothesis to discard this example, and try to pick different examples as part of the quota. But it relies on the operators we’re testing – a bit icky.

alexwlchan · 2017-04-10T10:03:43Z

ACK: I’ve seen this and will review it, but I probably won’t have time for a proper review until later this week.

Lukasa · 2017-04-10T10:04:12Z

❤️ Thanks @alexwlchan.

alexwlchan

Okay, looks good so far! I’d like to see the strategy tweaked to produce only valid settings objects, but it seems like a sensible approach.

I’m not convinced adapting the instance() and anotherInstance() methods are the right way to go here – producing two objects that are guaranteed distinct will be tricky, particularly because Hypothesis’s shrinking will tend them both to the same point if there’s a failure.

One way I’ve done equality testing in the past is to switch what the test does based on whether we have equal or distinct objects. Something like:

@given(SettingsStrategy)
def test_equality(x):
    assert x == x
    assert not (x != x)

@given(SettingsStrategy, SettingsStrategy)
def test_equality_operators(x, y):
    if x == y:
        assert x == y
        assert not (x != y)
    else:
        assert x != y
        assert not (x == y)

The second test usually produces two objects that are unequal, so we have the first test to counter it out. Not quite ideal, but it’s worked pretty well in the past.

alexwlchan · 2017-04-10T06:15:22Z

test/test_settings.py

+                h2.settings.SettingCodes.INITIAL_WINDOW_SIZE,
+                h2.settings.SettingCodes.MAX_FRAME_SIZE,
+                h2.settings.SettingCodes.MAX_CONCURRENT_STREAMS,
+                h2.settings.SettingCodes.MAX_FRAME_SIZE,


You’ve got MAX_FRAME_SIZE twice.

alexwlchan · 2017-04-12T14:50:32Z

test/test_settings.py

@@ -333,7 +333,7 @@ def test_cannot_set_invalid_values_for_max_frame_size(self, val):
            assert e.value.error_code == h2.errors.ErrorCodes.PROTOCOL_ERROR
            assert s[h2.settings.SettingCodes.MAX_FRAME_SIZE] == 16384

-    @given(integers())
+    @given(strategies.integers())


It makes no difference, so I’d prefer to change it back to keep the diff focused. 🙂

alexwlchan · 2017-04-12T14:58:19Z

test/test_settings.py

        assert not (a != b)

-    def test_different_eq(self):
+    @given(a=SettingsStrategy, b=SettingsStrategy)


I’m not sure Hypothesis has an easy way to generate two objects that are guaranteed to be distinct – any way we do that would rely on the equality operators, which are precisely what we’re trying to test!

Something like:

from hypothesis import assume @given(SettingsStrategy, SettingsStrategy) def test_equality_operators(x, y): assume(x != y): assert not (x == y)

Here assume will cause Hypothesis to discard this example, and try to pick different examples as part of the quota. But it relies on the operators we’re testing – a bit icky.

alexwlchan · 2017-04-12T15:30:25Z

test/test_settings.py

+        ) 
+
+    @given(o=SettingsStrategy)
+    def test_identical_eq(self, o):


Also, a minor thing: can we pick variable names that are a bit more descriptive please? 😄

fredthomsen · 2017-05-13T20:14:42Z

Believe all your comments are addressed now.

alexwlchan

Thanks, this is looking pretty good now.

I realise I misled you somewhat with a previous comment – apologies. Hopefully just a minor fix, not a major change. Otherwise a few minor things, but this feels pretty close to done now.

alexwlchan · 2017-05-13T22:09:58Z

test/test_settings.py

+        h2.settings.Settings,
+        client=booleans(),
+        initial_values=fixed_dictionaries({
+            h2.settings.SettingCodes.HEADER_TABLE_SIZE: integers(0),


Hypothesis also has a just() strategy, which returns a single value every time. Mildly more efficient, IIRC, although I don't think it makes a massive difference.

Not sure I understand where you want to apply the just strategy here. If you are referring to this specific line, the value listed here is just the minimum value for the integers strategy.

Oh yeah, oops, I was misreading integers(0) as integers(min_size=0, max_size=0).

Ignore me.

alexwlchan · 2017-05-13T22:11:20Z

test/test_settings.py

+            h2.settings.SettingCodes.ENABLE_PUSH: integers(0, 1),
+            h2.settings.SettingCodes.INITIAL_WINDOW_SIZE:
+                integers(0, 2**31 - 1),
+            h2.settings.SettingCodes.MAX_FRAME_SIZE: integers(16384, 16777215),


Maybe replace these two with the constants they're derived from? i.e.

h2.settings.SettingCodes.MAX_FRAME_SIZE: integers(2**14, 2**24 - 1)

alexwlchan · 2017-05-13T22:12:12Z

test/test_settings.py


-    def test_different_ne(self):
+    @given(settings=SettingsStrategy, o_settings=SettingsStrategy)
+    def test_equality_multiple(self, settings, o_settings):


Maybe other_settings? We’re not tight for characters.

alexwlchan · 2017-05-13T22:18:06Z

test/test_settings.py

-        b = self.another_instance()
-        assert not (a == b)
+        assert (settings == settings)
+        assert not (settings != settings)


I’ve realised since writing my last comment that this isn’t always the best test, because you’re comparing the identity of two identical objects. Python will default to using identity as an equality test, so this passes even if you don't supply custom equality methods:

>>> class Foo: pass ... >>> x = Foo() >>> y = Foo() >>> x == x True >>> x == y False >>> x != x False

My preferred approach is now to test based on copies of objects, i.e.,

other_settings = copy.deepcopy(settings) assert (settings == other_settings) assert not (settings != other_settings)

although this does rely on you being able to copy your objects, and I can’t remember what the options are here for IntEnum and its subclasses.

Ok, then it seems like this test makes little sense then as we have another where two strategies are generated, so I'll just nix this one.

alexwlchan · 2017-05-13T22:20:02Z

test/test_settings.py

-        assert (a != b) == [b]
+        delg = Delegate()
+        assert (settings == delg) == [delg]
+        assert (settings != delg) == [delg]


I realise we nicked this test from elsewhere, but would it be better to tweak the return values from __eq__ and __ne__ so we can be sure we're really calling the right method?

e.g.

def __eq__(self, other): return [self] def __ne__(self, other): return [self, self]

How embarrassing 😁 . Yes definitely.

Why is this worthy of it's own test btw? Can you enlighten me how this is not a given?

Err…

This comes from the PyOpenSSL test suite, a module which used to have a lot more C in its core lib than Python, so these things weren’t generally a given. I’m not sure it’s as useful for hyper-h2, and wouldn’t object if you chose to remove it.

alexwlchan · 2017-05-13T22:20:23Z

CONTRIBUTORS.rst

@@ -111,4 +111,5 @@ In chronological order:
 - Fred Thomsen (@fredthomsen)

  - Added logging.
-
+  - Enchance equality testing of ``h2.settings.Settings`` objects with


/s/Enchance/Enhance

Still probably want to deal with this. 😁

alexwlchan

Wasn't there a test that asserted an object (or its copy) was equal to itself? I think that's still a useful test to have – in practice, the current test_equality will almost always go down the not-equal branch.

alexwlchan · 2017-05-15T06:54:49Z

test/test_settings.py

+        h2.settings.Settings,
+        client=booleans(),
+        initial_values=fixed_dictionaries({
+            h2.settings.SettingCodes.HEADER_TABLE_SIZE: integers(0),


Oh yeah, oops, I was misreading integers(0) as integers(min_size=0, max_size=0).

Ignore me.

fredthomsen · 2017-05-15T13:11:44Z

@alexwlchan Yes, and to your point I restored that test. As you said, it will rarely go down that path of being equal when there are multiple settings generated from the strategy.

alexwlchan

Sorry for the somewhat slow reviews on this, but this looks really good now, thanks!

@Lukasa I don’t think I have any other comments on this patch – happy to merge if you are.

😄

Lukasa · 2017-05-16T22:35:33Z

Cool, shall we bring it up to date with the current master first?

fredthomsen · 2017-05-17T12:50:52Z

Branch has been rebased onto latest master.

Lukasa

One very small note, and then this looks ok!

Lukasa · 2017-05-17T17:08:31Z

test/test_settings.py

+            h2.settings.SettingCodes.MAX_FRAME_SIZE:
+                integers(2**14, 2**24 - 1),
+            h2.settings.SettingCodes.MAX_CONCURRENT_STREAMS: integers(0),
+            h2.settings.SettingCodes.MAX_HEADER_LIST_SIZE: integers(0),


Are we worried about creating values outside the valid range of some of these settings? These settings do have upper limits on their valid values.

Yes. I looked at the specs and saw that these are 32 bit fields and thus have adjusted them accordingly.

In this case we should update the _validate_settings function as well.

fredthomsen · 2017-05-28T22:19:11Z

Fixed misspelling in contributors file. I had made that change earlier, but I must have inadvertently blown it away.

Lukasa

Alright, this looks great! Thanks!

alexwlchan · 2017-05-30T09:17:38Z

🎉

Lukasa requested changes Apr 10, 2017

View reviewed changes

alexwlchan reviewed Apr 12, 2017

View reviewed changes

alexwlchan reviewed May 13, 2017

View reviewed changes

fredthomsen changed the title ~~WIP: Hypothesis h2 settings equality~~ Hypothesis h2 settings equality May 15, 2017

alexwlchan reviewed May 15, 2017

View reviewed changes

alexwlchan approved these changes May 15, 2017

View reviewed changes

Lukasa requested changes May 17, 2017

View reviewed changes

fredthomsen added 2 commits May 28, 2017 17:32

Use hypothesis to test h2 settings equality

054a3c2

Shameless plug

005d6fd

Lukasa approved these changes May 30, 2017

View reviewed changes

Lukasa merged commit e0adb86 into python-hyper:master May 30, 2017

Hypothesis h2 settings equality #543

Hypothesis h2 settings equality #543

Conversation

fredthomsen commented Apr 10, 2017

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexwlchan commented Apr 10, 2017

Lukasa commented Apr 10, 2017

alexwlchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredthomsen commented May 13, 2017

alexwlchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredthomsen May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexwlchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredthomsen commented May 15, 2017

alexwlchan left a comment

Choose a reason for hiding this comment

Lukasa commented May 16, 2017

fredthomsen commented May 17, 2017

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredthomsen commented May 28, 2017

Lukasa left a comment

Choose a reason for hiding this comment

alexwlchan commented May 30, 2017

fredthomsen May 15, 2017 •

edited

Loading