Opened 7 years ago

Closed 6 years ago

#2781 closed defect (fixed)

Stats.do_polling should have direct tests

Reported by: jinmei Owned by: naokikambe
Priority: medium Milestone: Sprint-20130903
Component: statistics Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 6 Internal?: no

Description

It is currently only tested indirectly. It should have direct
unit tests. I also suggest considering breaking it into a several
methods. It seems too big and does multiple things, and therefore
is not very understandable.

But in any case, please do not introduce any more reliance on threads.
This task should be done after #2689.

Subtickets

Change History (10)

comment:1 Changed 7 years ago by naokikambe

  • Milestone set to Next-Sprint-Proposed

comment:2 Changed 7 years ago by naokikambe

  • Owner set to naokikambe
  • Status changed from new to accepted

picking

comment:3 Changed 7 years ago by naokikambe

  • Milestone set to Next-Sprint-Proposed
  • Owner changed from naokikambe to UnAssigned
  • Status changed from accepted to reviewing

trac2781 is ready for reviewing.

  • break do_polling() into several methods
  • add unit tests for the broken methods
  • some unrelated changes

comment:4 Changed 7 years ago by naokikambe

I've pushed implementation to skip polling to the branch. If CC Session times out, stats gives up polling and keep running instead of falling down. Also log messages to warn it are added.

Last edited 7 years ago by naokikambe (previous) (diff)

comment:5 Changed 7 years ago by muks

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

comment:6 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to naokikambe

Hello.

Why does this code use while and removing items from the list instead of for? And, even if while would be needed, the len is not needed, you can use a list in a boolean expression and it is true if non-empty.

        _sequences = sequences[:]
        while len(_sequences) > 0:
            (module_name, seq) = (None, None)
            try:

This description doesn't match what is actually being done, because there's assertRaises at the end, not assertEqual([], …).

    def test_get_multi_module_list_initsessiontimeout(self):
        """Test _get_multi_module_list() returns an empty list if rcp_call()
        raise a InitSeeionTimeout exception"""

This class seems odd. Is there a reason why the method is created by lambda and not by def? And, why it isn't actually just a dict with data and a complete class is needed?

        class DummyDict:
            items = lambda x: [
                ('Init', 'dummy'), ('Stats', 'dummy'), ('Auth', 'dummy'),
                ]

Also, it seems there's quite a lot of changes not directly related to the ticket. Not that I'd think the changes are for bad, but it might have been better to create a separate ticket for it. But I don't insist on doing that ex-post.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 6 years ago by naokikambe

  • Owner changed from naokikambe to vorner

Hello, thank you for reviewing.

Replying to vorner:

Why does this code use while and removing items from the list instead of for? And, even if while would be needed, the len is not needed, you can use a list in a boolean expression and it is true if non-empty.

        _sequences = sequences[:]
        while len(_sequences) > 0:
            (module_name, seq) = (None, None)
            try:

I've used for-loop instead. Could you look at this?

c347349 [2781] use for-loop instead of while-loop for avoiding from rewriting the iterator

This description doesn't match what is actually being done, because there's assertRaises at the end, not assertEqual([], …).

    def test_get_multi_module_list_initsessiontimeout(self):
        """Test _get_multi_module_list() returns an empty list if rcp_call()
        raise a InitSeeionTimeout exception"""

I mistook. I've corrected documentation and the code.

5207d19 [2781] correct description of test_get_multi_module_list_initsessiontimeout()

This class seems odd. Is there a reason why the method is created by lambda and not by def? And, why it isn't actually just a dict with data and a complete class is needed?

        class DummyDict:
            items = lambda x: [
                ('Init', 'dummy'), ('Stats', 'dummy'), ('Auth', 'dummy'),
                ]

I've defined it as a normal function. But it is needed because the order of the array should be preserved at that code.

3669c95 [2781] define items() in DummDict class instead of using lambda, update note

Also, it seems there's quite a lot of changes not directly related to the ticket. Not that I'd think the changes are for bad, but it might have been better to create a separate ticket for it. But I don't insist on doing that ex-post.

As you know, do_polling() had various tasks as asking Init each module information, asking each module statistics, and collecting statistics. I think the scope of this ticket has become broad. Then some of changes I made might become things not directly related to this ticket. If you would like to have specific changes to be reviewed in separate tickets, please point out.

Regards,

comment:9 in reply to: ↑ 8 Changed 6 years ago by vorner

  • Owner changed from vorner to naokikambe

Hello

Replying to naokikambe:

As you know, do_polling() had various tasks as asking Init each module information, asking each module statistics, and collecting statistics. I think the scope of this ticket has become broad. Then some of changes I made might become things not directly related to this ticket. If you would like to have specific changes to be reviewed in separate tickets, please point out.

That's OK, no need to split to other tickets (which would be probably more work presently at doing the review as it is). I was just pointing it out to watch out for it in future.

Anyway, it looks OK to merge.

Thank you

comment:10 Changed 6 years ago by naokikambe

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 6

I'll keep it in mind. Anyway I've merged the branch and am closing the ticket.

Thank you.

Note: See TracTickets for help on using tickets.