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

Drop unused unittest components from test helpers and replace tests with pytest-based ones #67

Merged
merged 58 commits into from Jan 6, 2018

Conversation

Projects
None yet
2 participants
@webknjaz
Member

webknjaz commented Dec 2, 2017

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Cleanup

  • What is the related issue number (starting with #)
    #42

  • What is the current behavior? (You can also link to an open issue here)
    TerseTestResult, TerseTestRunner and ReloadingTestLoader exist.

  • What is the new behavior (if this is a feature change)?
    Drop above.

  • Other information:

@webknjaz webknjaz self-assigned this Dec 2, 2017

@webknjaz webknjaz requested a review from jaraco Dec 2, 2017

@webknjaz webknjaz force-pushed the feature/drop-unused-unittest-components branch from 4eba97d to 5ff6a72 Dec 14, 2017

@codecov

This comment has been minimized.

codecov bot commented Dec 14, 2017

Codecov Report

Merging #67 into master will decrease coverage by 43.99%.
The diff coverage is 12.3%.

@@            Coverage Diff            @@
##           master      #67     +/-   ##
=========================================
- Coverage   62.33%   18.34%    -44%     
=========================================
  Files          15       15             
  Lines        2963     2748    -215     
=========================================
- Hits         1847      504   -1343     
- Misses       1116     2244   +1128

@webknjaz webknjaz force-pushed the feature/drop-unused-unittest-components branch from 5ff6a72 to 5081f9c Dec 14, 2017

@webknjaz webknjaz changed the title from Drop unused unittest components from test helpers to WIP: Drop unused unittest components from test helpers Dec 14, 2017

@jaraco

This comment has been minimized.

Member

jaraco commented Dec 16, 2017

Although it's found in the 'tests' package, the webtest module in particular is part of the public interface of cheroot and cherrypy.

I did a quick search for ReloadingTestLoader, which turns up quite a few results. If there are users relying on this functionality, this change will need to describe what the recommendation is for those users. Maybe the recommendation is they adapt their tests to a new model--a fairly harsh ask. Or maybe we release this functionality in a separate package. I can't decide whether it's worth an intermediate release deprecating this functionality first.

In any case, I'm largely in favor of this change as long as we can make it without causing undue disruption.

config = {
'bind_addr': ('127.0.0.1', 54583),

This comment has been minimized.

@jaraco

jaraco Dec 16, 2017

Member

Here I'd like to see support for IPv6 and I'd like not to see hard-coded port numbers... which could conflict with other applications or other instances of the tests running. I suggest using portend.find_available_local_port or binding to the ephemeral port 0 and then resolving the bind port for the client.

This comment has been minimized.

@webknjaz

webknjaz Dec 17, 2017

Member

Yeah, I was thinking about it and prefer ephemeral port 0 approach as it's safer to defer port allocation to the OS and portend dependency is not even needed in this case.

I was trying to decouple existing unittest.TestCase based class into pieces, thus I didn't really take any steps to optimize or add new behavior at this stage. I didn't even replace the test suite module, but added new one because of this experimenting process.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Dec 17, 2017

I did a quick search for ReloadingTestLoader, which turns up quite a few results.

I followed the search and found out that all (or nearly all) of the results are just copies of cherrypy repo.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Dec 17, 2017

@jaraco

Do you think anyone uses/needs WebCase._handlewebError?

If yes, do you have any ideas on how to reimplement this using pytest?

@jaraco

This comment has been minimized.

Member

jaraco commented Dec 17, 2017

I don't know about _handlewebError, but I would recommend that in a pytest scenario, one shouldn't use _handlewebError, and the tests should just make assertions and let the test runner handle interactivity (such as through --pdb). Given that it's got the underscore prefix, I'd say it's sane to deprecate/remove it.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Dec 22, 2017

@jaraco well, it's kinda public, because depending on the environment variable (which I would consider to be a public interface) it intercepts assertion failures and provides an interactive interface for accessing/printing HTTP request/response values and suppressing/raising a real failures during test runs.

On the other hand I don't really see any need for this interface.

Also, I'd like to provide some new public pytest-oriented fixtures/helpers in cheroot.testing module rather that mixing them into cheroot.test.webtest

webknjaz added some commits Dec 2, 2017

Drop cheroot.test.helper.CherootWebCase.base()
It's not used in cheroot and is available in cherrypy copy of helper
Drop exit method from CherootWebCase and WebCase
cherrypy impl has it's own anyway

@webknjaz webknjaz force-pushed the feature/drop-unused-unittest-components branch from 436a16a to c7825a2 Dec 22, 2017

webknjaz added some commits Dec 23, 2017

Refactor socket info lookup in server.HTTPServer
Eliminated code duplication
Hotfix saving actual bound addr for ephemeral port
While this workaround would work, I'd prefer keeping both requested
and actual bind addrs within the server object, which might need bits
of redesign.

webknjaz added some commits Dec 29, 2017

@webknjaz webknjaz changed the title from WIP: Drop unused unittest components from test helpers to WIP: Drop unused unittest components from test helpers and replace tests with pytest-based ones Dec 29, 2017

@webknjaz webknjaz requested review from jaraco and cherrypy/contributors Dec 29, 2017

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jan 2, 2018

@jaraco could you plz share your opinion regarding public helpers for end-users (aka cheroot.testing)?

@jaraco

This comment has been minimized.

Member

jaraco commented Jan 3, 2018

I agree, creating an officially-supported testing module and separating it from a private tests package makes a lot of sense. Of course, doing so would be a backward-incompatible change, but we can manage that with deprecations and compatibility shims and iterative releases.

webknjaz added some commits Jan 3, 2018

@jaraco

Overall, this is great. I see lots to love in this change. I could have added a few nitpicks, but I decided to refrain so we can get this out and iterate on it. I did add a couple of comments I think are worthy of addressing in this PR, but nothing glaring. And tests work for me.

I haven't tested it against CherryPy. If you can confirm CherryPy tests pass with these changes, I'm prepared to rubberstamp!

@@ -1600,6 +1603,7 @@ def bind(self, family, type, proto=0):
pass
self.socket.bind(self.bind_addr)
self.bind_addr = self.socket.getsockname()[:2] # TODO: keep separate

This comment has been minimized.

@jaraco

jaraco Jan 6, 2018

Member

This change seems dangerous to me. You're overwriting the state on the object... so where one could previously access self.bind_addr to determine what address was attempted to be bound, now one gets instead the addr than was bound. Plus, there's a TODO - not sure why not just do:

@property
def bound_addr(self):
    return self.socket.getsockname()[:2]

or even better, just let the caller call server.socket.getsockname() directly.

This comment has been minimized.

@webknjaz

webknjaz Jan 6, 2018

Member

Yeah, it's just a dirty hack I did while refactoring tests. I wanted to postpone its resolution and to not dig into it while focusing on tests rewrite.
I don't want to mix this into the current PR, so I've logged this in #70

def _get_interrupt(self):
@property
def interrupt(self):
"""Return interrupt Exception instance."""

This comment has been minimized.

@jaraco

jaraco Jan 6, 2018

Member

This docstring is the one that will be present on the property, whereas previously the docstring would have provided indication about setting the exception. The docstring should be copied from the setter and the setter should probably have no meaningful docstring, maybe something like "setter for interrupt".

This comment has been minimized.

@webknjaz

webknjaz Jan 6, 2018

Member

Fair enough

This comment has been minimized.

@webknjaz

webknjaz Jan 6, 2018

Member

But still the docstring should describe the property/getter

This comment has been minimized.

@webknjaz

webknjaz Jan 6, 2018

Member

I'll try to rephrase that

'We are moving to pytest-based testing and thus deprecating '
'WEBTEST_INTERACTIVE environment variable support. '
'Interactive test failure interceptor is going to be removed '
'in Cheroot v7.0.0', # TODO: decide whether it's v7 or v8

This comment has been minimized.

@jaraco

jaraco Jan 6, 2018

Member

I've learned that it's difficult to predict when a feature will be dropped unless you have a project with long iterations and scheduled releases, neither of which apply to cheroot. I would simply say "WEBTEST_INTERACTIVE is deprecated." ...and in a future, backward-incompatible release, which we will determine when we get to it, we drop support.

@jaraco

jaraco approved these changes Jan 6, 2018

webknjaz added some commits Jan 6, 2018

Rephrase server.HTTPServer.interrupt docstring
Loosely addresses comment in PR by @jaraco. To be improved.
@webknjaz

This comment has been minimized.

Member

webknjaz commented Jan 6, 2018

@jaraco thanks for the review! 💫 🚀 🤖

@webknjaz webknjaz merged commit 0d0f18e into master Jan 6, 2018

2 of 7 checks passed

lgtm analysis: Python Analysis Failed (could not build the merge commit)
Details
ci/circleci Your tests failed on CircleCI
Details
codeclimate 3 issues to fix
Details
continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
WIP work in progress – do not merge!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@webknjaz

This comment has been minimized.

Member

webknjaz commented Jan 6, 2018

Confirmed that it's compatible with CherryPy

@webknjaz webknjaz changed the title from WIP: Drop unused unittest components from test helpers and replace tests with pytest-based ones to Drop unused unittest components from test helpers and replace tests with pytest-based ones Jan 7, 2018

@webknjaz webknjaz referenced this pull request Feb 3, 2018

Closed

Require client certificates #52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment