Skip to content

Commit

Permalink
Reworked mode tests.
Browse files Browse the repository at this point in the history 
This includes mode dictionary creation, removal of 'mode' member
from args_to_add dictionary and repositioning of the latter for
the sake of consistency.
  • Loading branch information
@Evildoor
Evildoor committed Nov 19, 2018
1 parent e1343c5 commit 1022253
Showing 1 changed file with 36 additions and 5 deletions.
41 changes: 36 additions & 5 deletions Utils/Dataflow/pyDKB/dataflow/stage/tests/test_ProcessorStage.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,20 @@
sys.exit(1)


args_to_add = {
'source': ['f', 's', 'h'],
'dest': ['f', 's', 'h'],
}


# mode: source, dest, EOM, EOP
modes = {
's': ['s', 's', '\n', '\0'],
'f': ['f', 'f', '\n', ''],
'm': ['s', 's', '\n', ''],
}


class ProcessorStageArgsTestCase(unittest.TestCase):
default_args = {
'mode': 'f',
Expand Down Expand Up @@ -103,11 +117,6 @@ def test_o(self):
self.check_args(args)


args_to_add = {
'source': ['f', 's', 'h'],
'dest': ['f', 's', 'h'],
'mode': ['f', 's', 'm'],
}


def add_arg(arg, val, short=False):
Expand All @@ -125,10 +134,32 @@ def f(self):
setattr(ProcessorStageArgsTestCase, 'test_%s_%s' % (arg, val), f)


def add_mode(val, short=False):
def f(self):
if short:
self.stage.parse_args(['-m', val])
else:
self.stage.parse_args(['--mode', val])
args = dict(self.default_args)
args['mode'] = val
args['source'] = modes[val][0]
args['dest'] = modes[val][1]
args['eom'] = modes[val][2]
args['eop'] = modes[val][3]

This comment has been minimized.

Sign in to view

Copy link
@mgolosova

mgolosova Nov 20, 2018

Collaborator

If (one day) we need to extend number of parametersm involved into the mode definition, we will have to change both modes hash and this piece of code. Or if we need more tests relying on the modes hash, they all will have to reproduce the 4 lines above (and be changed if/when the mode definition changed).

It could be simplified if done this way:

modes = {
  's': {'dest': 's', 'source': 's', 'eom': '\n', 'eop': '\0'},
  'f': {'dest': 'f', 'source': 'f', 'eom': '\n', 'eop': ''},
  'm': {'dest': 's', 'source': 's', 'eom': '\n', 'eop': ''}
}
...
def add_mode(val, short=False):
  def f(self):
    ...
    for param, p_val in modes[val].items():
      args[param] = p_val
    self.check_args(args)
    ...

Why:

  • makes things more readable (looking at modes you should not check the rest of the code to realize what the list of values means);
  • reduces risk of some mechanical mistakes (when one copy-and-pastes things and forgets to adjust index; when one mistakes the index value; etc.);
  • makes assignment part shotrer (2 lines, no matter how many parameters are defined by the mode);
  • makes it easier to extend 'mode' definition;
  • makes it easier to reuse modes hash.

Why not:

  • already works.

This "not" argument is Ok -- we need to make things working, first of all. But if you leave things "as is" for now, you should promise me that when you have to change the code / reuse modes, you will first make it more comfortable to use and to read.

This comment has been minimized.

Sign in to view

Copy link
@mgolosova

mgolosova Nov 20, 2018

Collaborator

Update:
Instead of

    for param, p_val in modes[val].items():
      args[param] = p_val

can also be used:

    args.update(modes[val])

And it looks even better, I believe.

This comment has been minimized.

Sign in to view

Copy link
@Evildoor

Evildoor Nov 20, 2018

Author Contributor

Yes, this makes sense and I'll take a look, now.

However, in such case I'm also suggesting to put this hash into pyDKB code (not now, in the future) instead of tests code (because the pyDKB code can use it as well) and import it in tests. That way it will be even simpler to make changes.
self.check_args(args)
if short:
setattr(ProcessorStageArgsTestCase, 'test_m_%s' % (val), f)
else:
setattr(ProcessorStageArgsTestCase, 'test_mode_%s' % (val), f)


for a in args_to_add:
for v in args_to_add[a]:
add_arg(a, v)
add_arg(a, v, True)
for m in modes:
add_mode(m)
add_mode(m, True)


test_cases = (
Expand Down

0 comments on commit 1022253

Please sign in to comment.