Refactor ipaddress module - #536
Conversation
Codecov Report
@@ Coverage Diff @@
## master #536 +/- ##
==========================================
+ Coverage 80.21% 80.35% +0.14%
==========================================
Files 21 22 +1
Lines 3699 3727 +28
==========================================
+ Hits 2967 2995 +28
Misses 732 732
Continue to review full report at Codecov.
|
|
I thought the Abstraction is for any test under a module rather than specifically for ip addr test. |
|
I'm not sure what you mean. There's two things going on -- the abstraction
itself which is only loosely applied (since e.g. there is no actual base
class) and then the singular instantiation of it for ipaddr tests.
…On Tue, Jul 14, 2020 at 6:52 PM henry54809 ***@***.***> wrote:
I thought the Abstraction is for any test under a module rather than
specifically for ip addr test.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#536 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD6OZT4TXIK6N3ERJDTR3UDXFANCNFSM4O2A77OQ>
.
|
|
So a test class just like the existing docker test class rather than the current ipaddr test class just for IP addr test. Do you plan on having a different class for each existing test module, ping, switch nmap, etc?
|
|
I would imaging the (virtual) module class hierarchy something like:
- (TestModule)
- Docker
- (Native)
- (Inline)
- ipaddr
The things in () don't currently exist as actual code -- either because of
a collapsed abstraction (joys of ducktyping) or because they haven't been
written yet (Native). So, something like ping would not have a specific
class as long as it's instantiated as a docker container (how it currently
is) or a native module. Right now, I'm not aware of anything else that
would be an "inline" module. A native module would be very similar to a
docker module, in that the actual DAQ code for it would be only a wrapper
around an externally executed script.
…On Tue, Jul 14, 2020 at 7:27 PM henry54809 ***@***.***> wrote:
So a test class just like the existing docker test class rather than the
current ipaddr test class just for IP addr test. Do you plan on having a
different class for each existing test module, ping, switch nmap, etc?
I'm not sure what you mean. There's two things going on -- the abstraction
itself which is only loosely applied (since e.g. there is no actual base
class) and then the singular instantiation of it for ipaddr tests.
… <#m_3978690006750142282_>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#536 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD47RXTG4HXP2SLK55LR3UHX5ANCNFSM4O2A77OQ>
.
|
henry54809
left a comment
henry54809
left a comment
There was a problem hiding this comment.
First of all, we should clearly differentiate module vs test everywhere in DAQ. 1 module can have multi tests, but all tests should be contained in some module, so the relationship between module and test is 1 to many. I envision the hierarchy can be something like this
- (Module)
- Docker
- (Native/ Inline)
- (Test 1 extends Test)
- (Test 2 extends Test)
...
- (Test)
...
That difference aside, in this PR, ipaddr and docker test are on the same level as indicated in the new_test function, even though docker test is a module and ip addr test is a test in the ipaddr module. I think there can be more time invested in thinking / experimenting with the level of abstractions needed for the hybrid of docker + native tests before we do commit.
I would imaging the (virtual) module class hierarchy something like: - (TestModule) - Docker - (Native) - (Inline) - ipaddr The things in () don't currently exist as actual code -- either because of a collapsed abstraction (joys of ducktyping) or because they haven't been written yet (Native). So, something like ping would not have a specific class as long as it's instantiated as a docker container (how it currently is) or a native module. Right now, I'm not aware of anything else that would be an "inline" module. A native module would be very similar to a docker module, in that the actual DAQ code for it would be only a wrapper around an externally executed script.
…
On Tue, Jul 14, 2020 at 7:27 PM henry54809 @.***> wrote: So a test class just like the existing docker test class rather than the current ipaddr test class just for IP addr test. Do you plan on having a different class for each existing test module, ping, switch nmap, etc? I'm not sure what you mean. There's two things going on -- the abstraction itself which is only loosely applied (since e.g. there is no actual base class) and then the singular instantiation of it for ipaddr tests. … <#m_3978690006750142282_> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#536 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIEPD47RXTG4HXP2SLK55LR3UHX5ANCNFSM4O2A77OQ .
| ALLIED_TELESIS_X230 = 0; | ||
| CISCO_9300 = 1; | ||
| OVS_SWITCH = 2; | ||
| FAUX_SWITCH = 3; |
There was a problem hiding this comment.
There's a setup where there is no switch that can be controlled (it's OVS but not reachable)... and that's what this field indicated. but, this is not the right place to handle that, so I put a check in at a different layer to detect that connection and not allow the connect RPC call at all.
| self.devdir, test_name) | ||
| self.logger.debug('test_host start %s/%s', test_name, self._host_name()) | ||
| def _new_test(self, test_name): | ||
| clazz = ipaddr_test.IpAddrTest if test_name == 'ipaddr' else docker_test.DockerTest |
There was a problem hiding this comment.
So IpAddrTest is a single port toggle test inside the ipaddr module whereas the DockerTest is a module, since Host class should be host of modules, something is not aligned. I think there should also be the test abstraction and module abstraction(other than dockertest) in place first before the current port_toggle test class.
Ip addr test and DockerTest are also both named test which is why we should also spend some time clearly distinguish test and module in code when we have time for more refactoring.
There was a problem hiding this comment.
IpAddrTest and DockerTest are both "modules" in the sense they are a high-level entity that's managed by ConnectedHost. Both IpAddrTest and DockerTest, in turn, encapsulate individual line-item tests. There is an abstraction in place, but maybe you mean that it should be an explicit abstraction (e.g. with a named base-class or similar?)
|
Re: names. Yes, they are not right and should be fixed, but that doesn't
affect the underlying abstractions. There's a distinction between a python
module and a test-module (class that's instantiated and managed by
ConnectedHost)... I think you're conflating them when you say "IpAddrTest
is a single port toggle test inside the ipaddr module" -- There's the
python module ipaddr_test.py, containing the test-module "class
IpAddrTest", and then there's line-item tests inside of that (function
_dhcp_port_toggle_test).
…On Wed, Jul 15, 2020 at 12:04 PM henry54809 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In daq/host.py
<#536 (comment)>:
> @@ -629,10 +615,15 @@ def _device_aux_path(self):
os.makedirs(path)
return path
- def _docker_test(self, test_name):
- self.test_host = docker_test.DockerTest(self.runner, self.target_port,
- self.devdir, test_name)
- self.logger.debug('test_host start %s/%s', test_name, self._host_name())
+ def _new_test(self, test_name):
+ clazz = ipaddr_test.IpAddrTest if test_name == 'ipaddr' else docker_test.DockerTest
So IpAddrTest is a single port toggle test inside the ipaddr module
whereas the DockerTest is a module, since Host class should be host of
modules, something is not aligned. I think there should also be the test
abstraction and module abstraction(other than dockertest) in place first
before the current port_toggle test class.
Ip addr test and DockerTest are also both named test which is why we
should also spend some time clearly distinguish test and module in code
when we have time for more refactoring.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#536 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD3IH7AC3GS5VN7QCGLR3X4UDANCNFSM4O2A77OQ>
.
|
2 participants
Not done yet (still need to get all the tests to pass... aka make it actually work), but this is the base of the refactoring.