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

Strengthen interface checks in tests #1225

Merged
merged 3 commits into from
Jul 13, 2020
Merged
Changes from all commits
Commits
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
111 changes: 107 additions & 4 deletions traits/tests/test_interfaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@
""" Unit test case for testing interfaces and adaptation.
"""

import contextlib
import logging
import unittest

from traits import has_traits
from traits.api import (
HasTraits,
Adapter,
Expand All @@ -27,6 +30,7 @@
TraitError,
)
from traits.adaptation.api import reset_global_adaptation_manager
from traits.interface_checker import InterfaceError


class IFoo(Interface):
Expand Down Expand Up @@ -178,23 +182,52 @@ class Test(HasTraits):
def test_provides_one(self):
@provides(IFoo)
class Test(HasTraits):
pass
def get_foo(self):
return "foo_and_only_foo"

def get_average(self):
return 42

test = Test()
self.assertIsInstance(test, IFoo)
self.assertNotIsInstance(test, IAverage)

def test_provides_multi(self):
@provides(IFoo, IAverage, IList)
class Test(HasTraits):
pass
def get_foo(self):
return "test_foo"

def get_average(self):
return 42

def get_list(self):
return [42]

test = Test()
self.assertIsInstance(test, IFoo)
self.assertIsInstance(test, IAverage)
self.assertIsInstance(test, IList)

def test_provides_extended(self):
""" Ensure that subclasses of Interfaces imply the superinterface.
"""

@provides(IFooPlus)
class Test(HasTraits):
pass
def get_foo(self):
return "some_test_foo"

def get_foo_plus(self):
return "more_test_foo"

test = Test()
self.assertIsInstance(test, IFoo)
self.assertIsInstance(test, IFooPlus)

ta = TraitsHolder()
ta.foo_adapted_to = Test()
ta.foo_adapted_to = test
self.assertIs(ta.foo_adapted_to, test)

def test_provides_bad(self):
with self.assertRaises(Exception):
Expand All @@ -203,6 +236,53 @@ def test_provides_bad(self):
class Test(HasTraits):
pass

def test_provides_with_no_interface_check(self):

class Test(HasTraits):
# Deliberately _not_ implementing get_foo. This class
# should not pass an IFoo interface check.
pass

provides_ifoo = provides(IFoo)
with self.set_check_interfaces(0):
# Simulate application of the decorator
Test = provides_ifoo(Test)

test = Test()
self.assertIsInstance(test, IFoo)

def test_provides_with_interface_check_warn(self):

class Test(HasTraits):
# Deliberately _not_ implementing get_foo. This class
# should not pass an IFoo interface check.
pass

provides_ifoo = provides(IFoo)
with self.set_check_interfaces(1):
with self.assertLogs("traits", logging.WARNING):
# Simulate application of the decorator
Test = provides_ifoo(Test)

test = Test()
self.assertIsInstance(test, IFoo)

def test_provides_with_interface_check_error(self):

class Test(HasTraits):
# Deliberately _not_ implementing get_foo. This class
# should not pass an IFoo interface check.
pass

provides_ifoo = provides(IFoo)
with self.set_check_interfaces(2):
with self.assertRaises(InterfaceError):
# Simulate application of the decorator
Test = provides_ifoo(Test)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the assignment Test = necessary here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not. I'm surprised I didn't get a flake8 warning. I'll remove it, and the similar occurrence in test_provides_with_interface_check_warn.

Copy link
Member Author

Choose a reason for hiding this comment

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

Though actually, it may be worth adding explicit tests that in the warning case, we do end up with something that implements IFoo, and in the error case, we don't.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way sounds good to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. It turns out that the extra test I added failed: even though the application of provides_ifoo fails in this case, Test still gets registered as implementing IFoo! It doesn't seem worth fixing, for something we're deprecating anyway, but now I'm not sure whether to encode the behaviour in a test or just leave it out.

On balance, I think I'll keep the extra test, but I could be persuaded otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Extra tests added; unused variable is now used!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. It turns out that the extra test I added failed: even though the application of provides_ifoo fails in this case, Test still gets registered as implementing IFoo! It doesn't seem worth fixing, for something we're deprecating anyway, but now I'm not sure whether to encode the behaviour in a test or just leave it out.

Yep this is surprising, and I agree that this is not worth fixing.


test = Test()
self.assertIsInstance(test, IFoo)

def test_instance_adapt_no(self):
ta = TraitsHolder()

Expand Down Expand Up @@ -317,3 +397,26 @@ def test_instance_requires_provides(self):
provider = UndeclaredAverageProvider()
with self.assertRaises(TraitError):
ta.a_no = provider

@contextlib.contextmanager
def set_check_interfaces(self, check_interfaces_value):
"""
Context manager to temporarily set has_traits.CHECK_INTERFACES
to the given value.

Parameters
----------
check_interfaces_value : int
One of 0 (don't check), 1 (check and log a warning on interface
mismatch) or 2 (check and raise on interface mismatch).

Returns
-------
context manager
"""
old_check_interfaces = has_traits.CHECK_INTERFACES
has_traits.CHECK_INTERFACES = check_interfaces_value
try:
yield
finally:
has_traits.CHECK_INTERFACES = old_check_interfaces