Opened 7 years ago

Closed 7 years ago

#2823 closed defect (fixed)

complete removing threads from stats tests

Reported by: jinmei Owned by: jinmei
Priority: very high Milestone: Sprint-20130528
Component: statistics Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 14.9 Internal?: no

Description

This is a followup of #2689.

First read the analysis comments of #2689:
http://bind10.isc.org/ticket/2689#comment:3

For b10-stats.test.py, the remaining tests to be converted are:

  • test_init
  • test_commands
  • test_command_show
  • test_command_showchema
  • test_polling
  • test_polling2

I believe we can eliminate the need for threads like other cases: use
the SimpleStats mock class instead of the production stats.Stats,
and directly pass/inspect messages over the ModuleCCSession.

I guess the same approach can apply to b10-stats-httpd_test for things
that rely on threaded BaseModules. It (seemingly) uses threads also
for http protocol handling. If so, we need another mock or something
to eliminate; at this moment I'm not sure if it's easy or difficult.
If it makes the task too big, we can postpone that part further.

Once we remove the dependency on BaseModules, we should be able to
get rid of all of these things. At that point I suggest rename
SimpleStats (as it's not that simple any more).

I'd also suggest renaming file names by removing "b10-" for
consistency.

Subtickets

Change History (23)

comment:1 Changed 7 years ago by jinmei

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

comment:2 Changed 7 years ago by shane

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

comment:3 Changed 7 years ago by jinmei

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

comment:4 Changed 7 years ago by jinmei

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130423
  • Owner set to jinmei
  • Status changed from new to accepted

comment:5 follow-up: Changed 7 years ago by jinmei

trac2823 is ready for review.

Changes are big, but most part of them should be straightforward.

This branch mainly consists of these 3 parts:

  • 8250e03: removing threads from rest of stats_test. This is an independent change set.
  • 8250e03..0a89b92: revised stats-httpd_test so we won't have to use threads in most cases. The key is the SimpleStatsHttpd class added in utils.py. It's an httpd counterpart mock class of SimpleStats. With the existence of this mock, most changes to stats-httpd should be straightforward. I tried to add more detailed explanation in comments for trickier cases, so if there's anything unclean please first check the comments and commit log. It still uses threads for handling HTTP requests from a client. IMO that's bad unittest too, but this usage of thread is relatively less harmful in the context of this ticket, so I left it for now. I also noticed some sockets are not cleanly closed at the end of the tests, resulting in warnings from Python. I suspect this is also related to the use of threads, but cannot be sure. At least for now I'd be inclined to leave it open for the purpose of this ticket. There's one case I actually couldn't test myself:
        @unittest.skipUnless(lxml_etree, "skipping XML validation with XSD")
        def test_xml_validation_with_xsd(self):
    
    as I don't have the dependency library. But according to the test content I believe it should work independently from the changes of this branch.
  • The final three commits are pure cleanups. The size is big but should be trivial.

I don't think we need a changelog entry for this task.

comment:6 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:7 in reply to: ↑ 5 Changed 7 years ago by jinmei

Replying to jinmei:

update:

inclined to leave it open for the purpose of this ticket. There's
one case I actually couldn't test myself:

    @unittest.skipUnless(lxml_etree, "skipping XML validation with XSD")
    def test_xml_validation_with_xsd(self):

as I don't have the dependency library.

I've installed the lxml module and confirmed the test still passed
on the branch.

comment:8 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:9 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

The tests fail for me:

FAIL: test_do_GET (__main__.TestHttpHandler)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/bind10/bind10-2/bind10-20130221/_build/../src/bin/stats/tests/stats-httpd_test.py", line 309, in test_do_GET
    check_XML_URL_PATH()
  File "/var/tmp/bind10/bind10-2/bind10-20130221/_build/../src/bin/stats/tests/stats-httpd_test.py", line 304, in check_XML_URL_PATH
    self.assertEqual(attr['value'], str(value))
AssertionError: '1308759248.0' != '1308726848.0'
- 1308759248.0
?      --
+ 1308726848.0
?       ++

This testcase is somewhat useless, with the return at the start of it:

class TestOSEnv(unittest.TestCase):
    def test_osenv(self):
        """
        test for the environ variable "B10_FROM_SOURCE"
        "B10_FROM_SOURCE" is set in Makefile
        """
        return
        # test case having B10_FROM_SOURCE
        self.assertTrue("B10_FROM_SOURCE" in os.environ)
        self.assertEqual(stats.SPECFILE_LOCATION, \
                             os.environ["B10_FROM_SOURCE"] + os.sep + \
                             "src" + os.sep + "bin" + os.sep + "stats" + \
                             os.sep + "stats.spec")
        # test case not having B10_FROM_SOURCE
        path = os.environ["B10_FROM_SOURCE"]
        os.environ.pop("B10_FROM_SOURCE")
        self.assertFalse("B10_FROM_SOURCE" in os.environ)
        # import stats again
        imp.reload(stats)
        # revert the changes
        os.environ["B10_FROM_SOURCE"] = path
        imp.reload(stats)

Do statistics really do such a strange thing as here? Once the queries per zone are a list with names inside the items, once it is named set:

            'Auth': {'queries.udp': 4, 'queries.tcp': 6,
                     'queries.perzone': [
                    {'queries.udp': 8, 'queries.tcp': 10,
                     'zonename': 'test1.example'},
                    {'queries.udp': 6, 'queries.tcp': 8,
                     'zonename': 'test2.example'}],
                     'nds_queries.perzone': {
                    'test10.example': {'queries.udp': 8, 'queries.tcp': 10},
                    'test20.example': {'queries.udp': 6, 'queries.tcp': 8}}}}

What is the purpose of this test? Does it really test that the test code throws an exception?

     def test_init_hterr(self):
        class FailingStatsHttpd(SimpleStatsHttpd):
            def open_httpd(self):
                raise stats_httpd.HttpServerError
        self.assertRaises(stats_httpd.HttpServerError, FailingStatsHttpd)

Also, there's lot of code snippets like:

self.assertEqual(None, something)

Would it be better to use

self.assertIsNone(something)

The same with assertIsNotNone.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by jinmei

Thanks for the review.

Replying to vorner:

The tests fail for me:

...

AssertionError?: '1308759248.0' != '1308726848.0'

  • 1308759248.0

? --
+ 1308726848.0
? ++
}}}

Oops, it's because specific local time was hardcoded. I believe it
was fixed in the latest version.

This testcase is somewhat useless, with the return at the start of it:

class TestOSEnv(unittest.TestCase):
    def test_osenv(self):
        """
        test for the environ variable "B10_FROM_SOURCE"
        "B10_FROM_SOURCE" is set in Makefile
        """
        return

...

}}}

My bad, it was a leftover. Removed, and the test should still pass.

Do statistics really do such a strange thing as here? Once the queries per zone are a list with names inside the items, once it is named set:

            'Auth': {'queries.udp': 4, 'queries.tcp': 6,
                     'queries.perzone': [
                    {'queries.udp': 8, 'queries.tcp': 10,
                     'zonename': 'test1.example'},
                    {'queries.udp': 6, 'queries.tcp': 8,
                     'zonename': 'test2.example'}],
                     'nds_queries.perzone': {
                    'test10.example': {'queries.udp': 8, 'queries.tcp': 10},
                    'test20.example': {'queries.udp': 6, 'queries.tcp': 8}}}}

This is basically not specific to this branch (so I don't think I'm
the right person to answer), but my guess is these are artificial
test-only spec and data. In fact, the real Auth module doesn't (yet)
even support per zone statistics at all. It's probably better to
avoid reusing the existing module for tests like this, but that would
be beyond the scope of this task. I at least clarified the (seeming)
intent of the unrealistic spec and data in comments.

What is the purpose of this test? Does it really test that the test code throws an exception?

     def test_init_hterr(self):
        class FailingStatsHttpd(SimpleStatsHttpd):
            def open_httpd(self):
                raise stats_httpd.HttpServerError
        self.assertRaises(stats_httpd.HttpServerError, FailingStatsHttpd)

Hmm, on a closer look, the original test (implicitly) checks one more
thing (in an expensive way): stats-httpd should tell ConfigMgr of its
departure (by send_stopping()). I believe it's sufficient to check
if close_mccs() is called, so I revised the test so it checks this
condition, too.

Also, there's lot of code snippets like:

self.assertEqual(None, something)

Would it be better to use

self.assertIsNone(something)

The same with assertIsNotNone.

Changed them as many as I can find.

comment:11 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:12 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

What is the purpose of this test? Does it really test that the test code throws an exception?

     def test_init_hterr(self):
        class FailingStatsHttpd(SimpleStatsHttpd):
            def open_httpd(self):
                raise stats_httpd.HttpServerError
        self.assertRaises(stats_httpd.HttpServerError, FailingStatsHttpd)

Hmm, on a closer look, the original test (implicitly) checks one more
thing (in an expensive way): stats-httpd should tell ConfigMgr of its
departure (by send_stopping()). I believe it's sufficient to check
if close_mccs() is called, so I revised the test so it checks this
condition, too.

The test was failing for me, see the commit in the branch for details.

If you agree, I think this can be merged.

comment:13 Changed 7 years ago by vorner

  • Total Hours changed from 0 to 3.65

comment:14 in reply to: ↑ 12 Changed 7 years ago by jinmei

Replying to vorner:

What is the purpose of this test? Does it really test that the test code throws an exception?

[...]

Hmm, on a closer look, the original test (implicitly) checks one more
thing (in an expensive way): stats-httpd should tell ConfigMgr of its
departure (by send_stopping()). I believe it's sufficient to check
if close_mccs() is called, so I revised the test so it checks this
condition, too.

The test was failing for me, see the commit in the branch for details.

If you agree, I think this can be merged.

Thanks, the change is okay for me (I'm not sure why it worked in my
environment though). I also made one last-minute change directly
to master: d2861ef. I believe it's sufficiently minor to have
explicit review, but say so if you think it has an issue.

Closing the ticket for now anyway.

comment:15 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 3.65 to 14.85

comment:16 Changed 7 years ago by jinmei

several regressions were reported, so I'm reopening the ticket.

I believe I've fixed all known regressions, and the fixes are
available at the trac2823-regression branch. Please review.

comment:17 Changed 7 years ago by jinmei

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:18 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from reopened to reviewing

comment:19 Changed 7 years ago by jinmei

  • Priority changed from medium to very high

comment:20 Changed 7 years ago by muks

  • Owner changed from UnAssigned to muks

Picking for review as this is causing several buildbot failures.

comment:21 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

I have pushed a couple of minor comment updates. Please check if they are OK.

This branch is OK to merge.

comment:22 in reply to: ↑ 21 Changed 7 years ago by jinmei

Replying to muks:

I have pushed a couple of minor comment updates. Please check if they are OK.

This branch is OK to merge.

Thanks for the review, merge done, (re-)closing.

comment:23 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 14.85 to 14.9
Note: See TracTickets for help on using tickets.