Opened 8 years ago

Closed 8 years ago

#1243 closed defect (fixed)

stats test shouldn't have its own "isc" packages

Reported by: jinmei Owned by:
Priority: medium Milestone: Year 3 Task Backlog
Component: statistics Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: High
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

There's a directory src/bin/stats/tests/isc, and some forked
versions of "isc" python packages (cc, config, log, util) are defined
beneath it.

IMO, this is a very bad approach.

First off, this is very confusing. Unless you know the trick
beforehand, you'll easily encounter a strange failure in importing an
isc python package, and it would take long time to identify the cause.

Second, obviously, this approach isn't scalable and easily leads to
duplicate efforts. Every time we develop a new python package or
library on which the stats or its test depends, either directly or
indirectly, you'll need to make a copy or forwarder under its forked
version. In fact, there's already a complete copy of
isc/log/init.py (imagine what happens when the original changes),
and a nullified version of isc/util/process.py (imagine this file
contains 100 methods).

If I understand the intent correctly, the forked packages are to
define some mock classes to replace modules that are not easy to use
under the test environment (like ccsession, which would need network
communication). Python should already be flexible enough to allow
such on-the-fly replacement without using an ugly overriding like
this. Please seriously reconsider an alternative that would eliminate
the forked isc packages.

Sooner is better, so I suggest this to be included in the next sprint.

Subtickets

Change History (11)

comment:1 follow-up: Changed 8 years ago by naokikambe

I know that problem, and I'm working to resolve it on #1175. Actually
these ugly packages all have been removed on trac1175. Shall we consider
about this problem after trac1175 is merged with the master?

Best

comment:2 in reply to: ↑ 1 ; follow-up: Changed 8 years ago by jinmei

Replying to naokikambe:

I know that problem, and I'm working to resolve it on #1175. Actually
these ugly packages all have been removed on trac1175. Shall we consider
about this problem after trac1175 is merged with the master?

Works for me, but I'm afraid that may mean the branch for #1175 is too
big to review (as was often the case for statistics-related tasks).
If that is possibly the case, please consider separating it into two
sub tasks.

comment:3 in reply to: ↑ 2 Changed 8 years ago by naokikambe

Thank you, but to be discussed on #1175. I wrote a comment on #1175.

comment:4 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Year 3 Task Backlog

comment:5 Changed 8 years ago by jreed

Is this all fixed?

This introduced a new problem where the init.pyc files still exist (the init.py and Makefile.am are gone), so stats tests can't find cfgmgr and other modules. I removed the stale init.pyc files to fix this. Maybe a head's up email can be sent to warn others. Or maybe stats/tests/Makefile.am CLEANFILES can take care of it.

comment:6 Changed 8 years ago by jreed

The underscore underscore got changed by wiki markup to underline the word in my above comment.

comment:7 follow-up: Changed 8 years ago by naokikambe

I don't know why such __init__.pyc is still there. But the reason that stats/tests/isc is left there is my mistake. I missed removing that at #1175. I don't think that Makefile needs to take care of it. BTW I've already removed that at #1275, but that ticket isn't directly related to this. May I create another ticket for it?

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

Replying to naokikambe:

I don't know why such __init__.pyc is still there. But the reason that stats/tests/isc is left there is my mistake. I missed removing that at #1175. I don't think that Makefile needs to take care of it. BTW I've already removed that at #1275, but that ticket isn't directly related to this. May I create another ticket for it?

I suspect it's an intermediate leftover problem when we delete something
from the repo. I don't think we need to add a new rule to Makefile(.am)
just for this, but it would be nice to add to ChangeLog? or some other
documentation that one may need to clean up leftovers (such as the
stats/tests/isc directly) by hand if one is trying BIND 10 retrieved
from the repository (rather than from a fresh tar ball).

comment:9 Changed 8 years ago by naokikambe

Thank you for the good suggestion. But that may happen in any other situations. May we add such cleanup in some tips about building from the repository?

comment:10 Changed 8 years ago by jinmei

Although there seemed to be an open question I think we should simply
close this ticket at this stage. Will do so.

comment:11 Changed 8 years ago by jinmei

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.