Hello Jozef, please see my comments inline.
Wed, Sep 30, 2020 at 04:14:50PM CEST, jurbanov(a)redhat.com wrote:
From: Jozef Urbanovsky <jurbanov(a)redhat.com>
Signed-off-by: Jozef Urbanovsky <jurbanov(a)redhat.com>
---
lnst/RecipeCommon/PacketAssert.py | 27 +++++++++++++++++++++++++++
lnst/Tests/PacketAssert.py | 20 ++++++++++++++++++++
2 files changed, 47 insertions(+)
This actually look weird. We should rename the file under RecipeCommon to
avoid confusion. It's out of the scope of this patch set review but
we should do a follow up fix at some point.
diff --git a/lnst/RecipeCommon/PacketAssert.py b/lnst/RecipeCommon/PacketAssert.py
index 689250e..b42be74 100644
--- a/lnst/RecipeCommon/PacketAssert.py
+++ b/lnst/RecipeCommon/PacketAssert.py
@@ -4,6 +4,17 @@ from lnst.Tests import PacketAssert
from lnst.Common.LnstError import LnstError
class PacketAssertConf(object):
+ """
+ Class for the configuration of the :any:`PacketAssert` class
+
+ :param host: client in :any:`PingConf` object used to specify host for the ping test
+ :param iface: interface used for the ping test
+ :param p_filter: string representation of desired packets in dump
+ :param grep_for: string representation of the content of given packet in dump
+ :param p_min: minimum count of the packets to be found in the dump
+ :param p_max: maximum count of the packets to be found in the dump
+ :param promiscuous: toggle of promiscuous mode
Please take a look at my comments for Tests/PacketAssert.py about the
parameters explanation.
+ """
def __init__(self, host, iface, **kwargs):
self._host = host
self._iface = iface
@@ -42,9 +53,19 @@ class PacketAssertConf(object):
return self._promiscuous
class PacketAssertTestAndEvaluate(BaseRecipe):
+ """
+ Class to control and execute :any:`PacketAssert` test.
I think this should be enhanced a bit.
"
This class provides an extension to BaseRecipe class to perform an
evaluation of the captured packets on an interface. The class uses
:any:`PacketAssert` test module to capture the packets based on the filters
defined in :any:`PacketAssertConf` configuration. The pass or fail
decision is made upon whether the number of the captured packets
matching the criteria fits the min/max interval defined through
:any:`PacketAssertConf`.
"
+ """
started_job = None
def packet_assert_test_start(self, packet_assert_config):
+ """
+ Method starts a :any:`PacketAssert` job and stores ***started_job*** attribute
+ containing LNST :any:`Job: object.
+
+ :param packet_assert_config: configuration in a form of :any:`PacketAssertConf`
+ class
+ """
if self.started_job:
raise LnstError("Only 1 packet_assert job is allowed to run at a
time.")
@@ -54,6 +75,12 @@ class PacketAssertTestAndEvaluate(BaseRecipe):
self.started_job = host.prepare_job(packet_assert).start(bg=True)
def packet_assert_test_stop(self):
+ """
+ Method kills a process executing :any:`PacketAssert` job and
+ resets the value of the ***started_job*** attribute.
+
+ :return: result of the packet assert
+ """
if not self.started_job:
raise LnstError("No packet_assert job is running.")
Another method that should be documented here is
packet_assert_evaluate_and_report() as it is going to be used by a user.
diff --git a/lnst/Tests/PacketAssert.py b/lnst/Tests/PacketAssert.py
index 25a92ba..901fdc0 100644
--- a/lnst/Tests/PacketAssert.py
+++ b/lnst/Tests/PacketAssert.py
@@ -8,6 +8,19 @@ from lnst.Tests.BaseTestModule import BaseTestModule
from lnst.Common.LnstError import LnstError
class PacketAssert(BaseTestModule):
+ """
+ Class to control tcpdump test
+
+ This class runs tcpdump and searches through its output in order
+ to evaluate whether specified packets are traversing the network stack.
Few corrections and suggestions.
"Class to control tcpdump test" - er, what is the "tcpdump test"?
I'd
remove this completely.
I'd extend this a bit to give user an overview how this can be used and
why it could be useful for the user.
Something like:
"This test module utilizes tcpdump to capture packets on a network interface
based on filters defined by the test module parameters. It returns the number
of captured packets that match the filter criteria.
This test module is usually used in conjunction with :any:`Ping` test module,
for example:
pa_job = host1.run(PacketAssert())
host1.run(Ping())
pa_job.kill()
if pa_job.res_data['p_recv'] < 10:
print("failed")
"
Please update the code example as I made this up and I'm not sure about
the accuracy of the method/module names.
+
+ :param interface: interface used for the :any:`PacketAssert` test
I'd rather refer to tcpdump than the class itself.
+ :param p_filter: string representation of desired packets in dump
This is a tcpdump pcap filter (man tcpdump, man 7 pcap-filter).
+ :param grep_for: string representation of the content of given
packet in dump
This is a regular expression to be matched in the string representation of a
packet in the tcpdump output.
+ :param promiscuous: toggle of promiscuous mode
+ :param grep_exprs: concatenated expression to be used by grep for search
This is not a parameter exposed to user, please remove it.
To expose a TestModule class attribute as a parameter to user it has to be
an instance of *Param class.
+ :param p_recv: amount of received packets
This is not a parameter exposed to user, please remove it.
+ """
interface = DeviceParam(mandatory=True)
p_filter = StrParam(default='')
grep_for = ListParam(default=[])
@@ -57,6 +70,13 @@ class PacketAssert(BaseTestModule):
return False
def run(self):
+ """
+ Method used for the control of :any:`PacketAssert` test.
+ Process with tcpdump is spawned and its output is digested
+ in order to find and count specified packets through grep expressions.
+
+ :return: result of the run
So I think this could be probably removed as it's basically the same
that I proposed for the class docstring and IMO is a better place.
Just a note that ":return: result of the run" is too vague.
+ """
self._res_data = {}
if not is_installed("tcpdump"):
self._res_data["msg"] = "tcpdump is not installed on this
machine!"
--
2.25.4
_______________________________________________
LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedora...