[changes requested] Refactor cli_test.py and main_test.py by cowlicks · Pull Request #3828 · certbot/certbot

cowlicks · 2016-11-29T18:29:51Z

This moves the tests in cli_test.py that used the certbot.main.main entrypoint to main_test.py. It also moves a lot of the other extraneous tests out of there.

It does not fully close #2716 because many of the tests now in main_test.py still use the same monolithic entrypoint, but it is a good step toward that.

coveralls · 2016-11-29T18:41:18Z

Coverage increased (+0.003%) to 98.852% when pulling db406dd on cowlicks:gh-2716 into 7951ba7 on certbot:master.

pde · 2016-11-29T20:49:01Z

This looks like heroic work!

coveralls · 2016-11-29T21:37:46Z

Coverage increased (+0.002%) to 98.85% when pulling edd26b0 on cowlicks:gh-2716 into 7951ba7 on certbot:master.

ohemorange

This PR is fantastic, great work!

There's a new file not_cli_test.py that's probably not supposed to be there.

ohemorange · 2016-11-29T23:24:33Z

certbot/tests/error_handler_test.py

        self.assertFalse(self.init_func.called)

+    @mock.patch('certbot.main.sys')
+    def test_handle_exception(self, mock_sys):


It's confusing to me to have _handle_exception() defined in main.py but tested in error_handler_test.py. I don't know the history of why it's in main, but could we move it to error_handler.py to match the test?

I looks like the purpose of error_handler.ErrorHandler is to handle clean up of stateful operations when there is an exception. But main._handle_exception is for logging tracebacks and displaying them to the user properly. So they are separate but sound similar.

I think it would be better to move this test into its own class in main_test.py.

Either way works for me, as long as they match.

ohemorange · 2016-11-29T23:29:25Z

certbot/tests/main_test.py

+        self.assertEqual(result, (None, None))
+
+
+class MainTest(unittest.TestCase):  # pylint: disable=too-many-public-methods


How would you feel about having a BaseMainTest class that the different verb testers inherit from?

That sounds like a good strategy. But I think I will save breaking this into separate verb tests for another PR. This PR just gets the low hanging fruit, like the parsing tests.

Sounds good! I'll make you an issue for it.

ohemorange · 2016-11-29T23:31:20Z

certbot/tests/renewal_test.py

+import tempfile
+
+from certbot import configuration
+from certbot import renewal


Can you move this into the method, just before calling _restore_webroot_config?

ohemorange · 2016-11-29T23:32:38Z

certbot/tests/test_util.py

        return lambda cls: None
+
+
+def make_lineage(self, testfile):


With this method in test_util, does DuplicativeCertsTest have to inherit from BaseRenewableCertTest?

Yeah, it looks like make_lineage can be used instead of inheriting from BaseRenewableCertTest. I'll try that this afternoon.

I didn't finish this today, I'll take another look tomorrow.

@ohemorange, a few things for posterity, make_lineage creates as lineage based on any of the configuration files in certbot/tests/testdata/*.conf. While BaseRenewableCertTest makes a lineage (correct word?) on the fly, based on some parameters in its class definition. It would be nice to remove the dependency of BaseRenewableCertTest since it is imported from another test file.

After a few attempts I could not get the DuplicativeCertsTest tests to work using make_lineage. So I'll punt for now and make another issue.

ohemorange · 2016-11-29T23:34:44Z

Oh also this is going to conflict with #3785, so let's try to get this one in.

cowlicks · 2016-11-29T23:43:22Z

@pde @ohemorange I think I should rename test_util.py to util.py to avoid confusing with util_test.py. And the name information is already there in the import path certbot.tests.test_util. Any objections?

ohemorange · 2016-11-29T23:45:16Z

Only reason I see to keep it is that we might then get some import certbot.tests.util as test_util to avoid conflicts with the other util. No strong opinion either way.

cowlicks · 2016-11-29T23:56:04Z

@ohemorange cool, I'll probably have time to address your comments tomorrow morningish CST.

pde · 2016-12-01T22:44:47Z

Kicking Travis

cowlicks · 2016-12-02T19:07:26Z

certbot/tests/main_test.py

+        return functools.partial(cli.prepare_and_parse_args, plugins)
+
+    # why can't I remove this?
+    def test_option_was_set(self):


A lot of this refactoring expects that prepare_and_parse_args is stateless, but it actually has some global side effects which are tested here. Is there a better way to handle this state?

Fixed. I just manually reset the state of set_by_cli in each test. This is way cheaper and faster than loading all the plugins and rebuilding the parser for each test.

@ohemorange

from @ohemorange comments

cowlicks · 2016-12-03T01:56:14Z

Rebased

coveralls · 2016-12-03T01:56:46Z

Coverage increased (+0.001%) to 98.854% when pulling 47a78e5 on cowlicks:gh-2716 into da3332c on certbot:master.

coveralls · 2016-12-03T02:01:41Z

Coverage increased (+0.001%) to 98.854% when pulling 47a78e5 on cowlicks:gh-2716 into da3332c on certbot:master.

coveralls · 2016-12-03T20:09:19Z

Coverage decreased (-0.01%) to 98.842% when pulling f985cd8 on cowlicks:gh-2716 into da3332c on certbot:master.

coveralls · 2016-12-03T20:16:17Z

Coverage increased (+0.001%) to 98.854% when pulling f985cd8 on cowlicks:gh-2716 into da3332c on certbot:master.

coveralls · 2016-12-04T03:36:30Z

Coverage increased (+0.001%) to 98.854% when pulling 5d72e1a on cowlicks:gh-2716 into da3332c on certbot:master.

cowlicks · 2016-12-04T03:37:32Z

@ohemorange Anything else?

pde added the area: refactoring label Nov 29, 2016

pde assigned ohemorange Nov 29, 2016

pde added the area: testing label Nov 29, 2016

ohemorange requested changes Nov 29, 2016

View reviewed changes

pde assigned cowlicks and unassigned ohemorange Nov 30, 2016

pde changed the title ~~Refactor cli_test.py and main_test.py~~ [changes requested] Refactor cli_test.py and main_test.py Nov 30, 2016

ohemorange mentioned this pull request Nov 30, 2016

Break MainTest into BaseMainTest + class per verb #3832

Closed

pde closed this Dec 1, 2016

pde reopened this Dec 1, 2016

cowlicks closed this Dec 2, 2016

cowlicks reopened this Dec 2, 2016

cowlicks force-pushed the gh-2716 branch from 599f49c to 20415a6 Compare December 2, 2016 04:02

cowlicks mentioned this pull request Dec 2, 2016

Remove BaseRenewableCertTest dependency from DuplicativeCertsTest #3842

Closed

cowlicks commented Dec 2, 2016

View reviewed changes

cowlicks force-pushed the gh-2716 branch 2 times, most recently from 1b32cc1 to 47a78e5 Compare December 3, 2016 01:49

cowlicks added 5 commits December 2, 2016 17:51

Begin breaking out cli_test.py

ccac92a

simplify main

3981824

refactor porse tests

f30635b

move determine account tests to main_test.py

ab6e4ab

move duplicate cert test to main_test.py

6c065bc

cowlicks added 18 commits December 2, 2016 17:51

move cli stuff out of the way

2cfb87b

add test_renewal.py

e229250

move error test into error_handler_test.py

fba655a

move test_read_file

34a27f4

move test_no_gui out of MainTest

e9d56b8

move test_install_abspath to parsetest

873494f

Move main tests into main_test.py

6eb3294

move cli tests back into cli_test.py

32c25f0

clean up cli_test.py

a4550f3

move punycode test to util_test.py

c7e1cd3

Fix NameError from missing plugins_disco

2c79d1c

Fix linting errors

8e83e2e

test_renewal.py -> renewal_test.py

b65cfbb

rm not_cli_test.py

8128d11

Move main._handle_exception test to main_test.py

dfebbcb

Move renewal import in renewal_test.py

3f290ac

from @ohemorange comments

certbot.tests.test_util -> certbot.tests.util

c892319

Fix issues from rebasing.

47a78e5

cowlicks force-pushed the gh-2716 branch from b786aa1 to f985cd8 Compare December 3, 2016 20:03

Fix testing issue with option_was_set

f985cd8

fix linting issue

5d72e1a

ohemorange approved these changes Dec 5, 2016

View reviewed changes

ohemorange merged commit 65d9e99 into certbot:master Dec 5, 2016

		self.assertEqual(result, (None, None))


		class MainTest(unittest.TestCase): # pylint: disable=too-many-public-methods

Uh oh!

Conversation

cowlicks commented Nov 29, 2016

Uh oh!

coveralls commented Nov 29, 2016

Uh oh!

pde commented Nov 29, 2016

Uh oh!

coveralls commented Nov 29, 2016

Uh oh!

ohemorange left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ohemorange commented Nov 29, 2016

Uh oh!

cowlicks commented Nov 29, 2016

Uh oh!

ohemorange commented Nov 29, 2016

Uh oh!

cowlicks commented Nov 29, 2016

Uh oh!

pde commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cowlicks commented Dec 3, 2016

Uh oh!

coveralls commented Dec 3, 2016

Uh oh!

coveralls commented Dec 3, 2016

Uh oh!

coveralls commented Dec 3, 2016

Uh oh!

coveralls commented Dec 3, 2016

Uh oh!

coveralls commented Dec 4, 2016

Uh oh!

cowlicks commented Dec 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cowlicks commented Dec 4, 2016 •

edited

Loading