-
Notifications
You must be signed in to change notification settings - Fork 85
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
Conversation
I extended the PR to add some tests for the current interface-checking behaviour of the |
with self.set_check_interfaces(2): | ||
with self.assertRaises(InterfaceError): | ||
# Simulate application of the decorator | ||
Test = provides_ifoo(Test) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM
This PR enhances some of the tests in
test_interfaces.py
:CHECK_INTERFACES
is set to2
.)assertIsInstance
checks to verify that instances of those test classes do pass the appropriateisinstance
checks.