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.

Sign up for GitHub

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

Hypothesis h2 settings equality #543

Merged
merged 2 commits into from
May 30, 2017
Merged

Hypothesis h2 settings equality #543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
054a3c2
Use hypothesis to test h2 settings equality
fredthomsen Apr 10, 2017
005d6fd
Shameless plug
fredthomsen May 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CONTRIBUTORS.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,5 @@ In chronological order:
- Fred Thomsen (@fredthomsen)

- Added logging.

- Enhance equality testing of ``h2.settings.Settings`` objects with
``hypothesis``.
150 changes: 48 additions & 102 deletions test/test_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
import h2.settings

from hypothesis import given, assume
from hypothesis.strategies import integers
from hypothesis.strategies import (
integers, booleans, fixed_dictionaries, builds
)


class TestSettings(object):
Expand Down Expand Up @@ -367,125 +369,69 @@ class TestSettingsEquality(object):
A class defining tests for the standard implementation of == and != .
"""

def an_instance(self):
"""
Return an instance of the class under test. Each call to this method
must return a different object. All objects returned must be equal to
each other.
"""
overrides = {
h2.settings.SettingCodes.HEADER_TABLE_SIZE: 0,
h2.settings.SettingCodes.MAX_FRAME_SIZE: 16384,
h2.settings.SettingCodes.MAX_CONCURRENT_STREAMS: 4,
h2.settings.SettingCodes.MAX_HEADER_LIST_SIZE: 2**16,
}
return h2.settings.Settings(client=True, initial_values=overrides)

def another_instance(self):
"""
Return an instance of the class under test. Each call to this method
must return a different object. The objects must not be equal to the
objects returned by an_instance. They may or may not be equal to
each other (they will not be compared against each other).
"""
overrides = {
h2.settings.SettingCodes.HEADER_TABLE_SIZE: 8080,
h2.settings.SettingCodes.MAX_FRAME_SIZE: 16388,
h2.settings.SettingCodes.MAX_CONCURRENT_STREAMS: 100,
h2.settings.SettingCodes.MAX_HEADER_LIST_SIZE: 2**16,
}
return h2.settings.Settings(client=False, initial_values=overrides)

def test_identical_eq(self):
"""
An object compares equal to itself using the == operator.
"""
o = self.an_instance()
assert (o == o)

def test_identical_ne(self):
"""
An object doesn't compare not equal to itself using the != operator.
"""
o = self.an_instance()
assert not (o != o)

def test_same_eq(self):
"""
Two objects that are equal to each other compare equal to each other
using the == operator.
"""
a = self.an_instance()
b = self.an_instance()
assert (a == b)

def test_same_ne(self):
"""
Two objects that are equal to each other do not compare not equal to
each other using the != operator.
"""
a = self.an_instance()
b = self.an_instance()
assert not (a != b)
SettingsStrategy = builds(
h2.settings.Settings,
client=booleans(),
initial_values=fixed_dictionaries({
h2.settings.SettingCodes.HEADER_TABLE_SIZE:
integers(0, 2**32 - 1),
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(2**14, 2**24 - 1),
h2.settings.SettingCodes.MAX_CONCURRENT_STREAMS:
integers(0, 2**32 - 1),
h2.settings.SettingCodes.MAX_HEADER_LIST_SIZE:
integers(0, 2**32 - 1),
})
)

def test_different_eq(self):
@given(settings=SettingsStrategy)
def test_equality_reflexive(self, settings):
"""
Two objects that are not equal to each other do not compare equal to
each other using the == operator.
An object compares equal to itself using the == operator and the !=
operator.
"""
a = self.an_instance()
b = self.another_instance()
assert not (a == b)
assert (settings == settings)
assert not (settings != settings)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


def test_different_ne(self):
@given(settings=SettingsStrategy, o_settings=SettingsStrategy)
def test_equality_multiple(self, settings, o_settings):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe other_settings? We’re not tight for characters.

"""
Two objects that are not equal to each other compare not equal to each
other using the != operator.
Two objects compare themselves using the == operator and the !=
operator.
"""
a = self.an_instance()
b = self.another_instance()
assert (a != b)
if settings == o_settings:
assert settings == o_settings
assert not (settings != o_settings)
else:
assert settings != o_settings
assert not (settings == o_settings)

def test_another_type_eq(self):
@given(settings=SettingsStrategy)
def test_another_type_equality(self, settings):
"""
The object does not compare equal to an object of an unrelated type
(which does not implement the comparison) using the == operator.
"""
a = self.an_instance()
b = object()
assert not (a == b)

def test_another_type_ne(self):
"""
The object compares not equal to an object of an unrelated type (which
does not implement the comparison) using the != operator.
"""
a = self.an_instance()
b = object()
assert (a != b)
obj = object()
assert (settings != obj)
assert not (settings == obj)

def test_delegated_eq(self):
@given(settings=SettingsStrategy)
def test_delegated_eq(self, settings):
"""
The result of comparison using == is delegated to the right-hand
operand if it is of an unrelated type.
The result of comparison is delegated to the right-hand operand if
it is of an unrelated type.
"""
class Delegate(object):
def __eq__(self, other):
return [self]

a = self.an_instance()
b = Delegate()
assert (a == b) == [b]

def test_delegate_ne(self):
"""
The result of comparison using != is delegated to the right-hand
operand if it is of an unrelated type.
"""
class Delegate(object):
def __ne__(self, other):
return [self]

a = self.an_instance()
b = Delegate()
assert (a != b) == [b]
delg = Delegate()
assert (settings == delg) == [delg]
assert (settings != delg) == [delg]
Copy link
Contributor

Choose a reason for hiding this comment

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

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]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How embarrassing 😁 . Yes definitely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.