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

Remove obsolete call of searchInterface from interfaceToName #33

Merged
merged 1 commit into from
Sep 16, 2017

Conversation

pbauer
Copy link
Member

@pbauer pbauer commented Sep 3, 2017

Fixes #32

@pbauer pbauer requested a review from tseaver September 4, 2017 05:52
@tseaver
Copy link
Member

tseaver commented Sep 4, 2017

I have no problem with the substance of this change, but there should be a test failure: coveralls reports we are at 100% before this PR, which doesn't remove any tests but does remove lines that were presumably covered.

@jamadden You're the one responsible for switching from tox -e coverage to coveralls to measure coverage: can you explain?

@pbauer
Copy link
Member Author

pbauer commented Sep 5, 2017

In zopefoundation/zope.browserpage@b82f337 I use exactly the same pattern as the original issue in Plone: A browsermenu is registered with id and interface:

<browser:menu
  id="test_menu"
  title="Test menu"
  interface="zope.browserpage.tests.test_page.ITestMenu"/>

When using interfaceToName on that interface an Error is raised: https://travis-ci.org/zopefoundation/zope.browserpage/builds/271922007

@mgedmin
Copy link
Member

mgedmin commented Sep 5, 2017

Speaking of coverage, well, we don't have branch coverage enabled, do we? And it's not surprising that an assert statement that gets executed with the condition evaluating to False is marked as covered.

@jamadden
Copy link
Member

jamadden commented Sep 5, 2017

Right, branch coverage is not enabled, so we just had to take both the if branch (which a search for one registration would do) and the alternative (which a search for nothing registered would do). Given that both alternatives produce the same thing (interface.__module__ + '.' + interface.__name__), they cannot be distinguished from tests. The test case that was missing was one that intentionally set things up to fail (searching for multiple registrations).

@pbauer
Copy link
Member Author

pbauer commented Sep 5, 2017

@jamadden are you saying that the pattern I use above (setting up a browsermenu with id and interface) is wrong? It has been used in zope.browsermenu, I only added the failing test.

@jamadden
Copy link
Member

jamadden commented Sep 5, 2017

@pbauer I wasn't commenting on zope.browserpage, only on the tests in this package.

@tseaver
Copy link
Member

tseaver commented Sep 5, 2017

@mgedmin There is an actual (non-assert) branch in the removed code. How are the current tests exercising the early return? Any why don't they fail in this PR?

@jamadden
Copy link
Member

jamadden commented Sep 5, 2017

The current tests exercise exactly the two cases that the code handled: one registered interface and no registered interfaces. They don't test the failure case of multiple registered interfaces (why not? Probably because that was clearly not supposed to happen, given the presence of an assert). Note that the two cases result in the same text being returned, which is the text that's still being returned, so nothing fails when both cases become the same case.

@pbauer
Copy link
Member Author

pbauer commented Sep 15, 2017

Can this be merged or is something still missing? If so, what exactly?

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.

None yet

4 participants