Skip to content

Commit

Permalink
Added testing for arguments' interactions.
Browse files Browse the repository at this point in the history
  • Loading branch information
@Evildoor
Evildoor committed Nov 19, 2018
1 parent 1022253 commit 088e180
Showing 1 changed file with 97 additions and 0 deletions.
97 changes: 97 additions & 0 deletions Utils/Dataflow/pyDKB/dataflow/stage/tests/test_ProcessorStage.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ def test_hdfs(self):
self.stage.parse_args(['--hdfs'])
args = dict(self.default_args)
args['hdfs'] = True
args['source'] = 'h'
args['dest'] = 'h'
self.check_args(args)

def test_eom(self):
Expand Down Expand Up @@ -116,7 +118,54 @@ def test_o(self):
args['output_dir'] = 'something'
self.check_args(args)

# hdfs >> source-dest >> mode
def test_override_hdfs_m_s(self):
self.stage.parse_args(['-m', 's', '--hdfs'])
args = dict(self.default_args)
args['hdfs'] = True
args['source'] = 'h'
args['dest'] = 'h'

This comment has been minimized.

Sign in to view

Copy link
@mgolosova

mgolosova Nov 20, 2018

Collaborator

Should not the other mode-related arguments be added to args? Else both EOP and EOM are left default values (here and in other test_override_hdfs_m[ode]_*).

This comment has been minimized.

Sign in to view

Copy link
@Evildoor

Evildoor Nov 20, 2018

Author Contributor

Have not thought about that, thanks for noticing.
self.check_args(args)

def test_override_hdfs_m_f(self):
self.stage.parse_args(['-m', 'f', '--hdfs'])
args = dict(self.default_args)
args['hdfs'] = True
args['source'] = 'h'
args['dest'] = 'h'
self.check_args(args)

def test_override_hdfs_m_m(self):
self.stage.parse_args(['-m', 'm', '--hdfs'])
args = dict(self.default_args)
args['hdfs'] = True
args['source'] = 'h'
args['dest'] = 'h'
self.check_args(args)

def test_override_hdfs_mode_s(self):
self.stage.parse_args(['--mode', 's', '--hdfs'])
args = dict(self.default_args)
args['hdfs'] = True
args['source'] = 'h'
args['dest'] = 'h'
self.check_args(args)

def test_override_hdfs_mode_f(self):
self.stage.parse_args(['--mode', 'f', '--hdfs'])
args = dict(self.default_args)
args['hdfs'] = True
args['source'] = 'h'
args['dest'] = 'h'
self.check_args(args)

def test_override_hdfs_mode_m(self):
self.stage.parse_args(['--mode', 'm', '--hdfs'])
args = dict(self.default_args)
args['hdfs'] = True
args['source'] = 'h'
args['dest'] = 'h'
self.check_args(args)

This comment has been minimized.

Sign in to view

Copy link
@mgolosova

mgolosova Nov 20, 2018

Collaborator

Isn`t it possible to wrap these methods definition into a loop, as it was done for other cases?
Something like:

def add_override_hdfs_mode(mode, short=False):
    def f(self):
        if short:
            self.stage.parse_args(['-m', mode, '--hdfs'])
        else:
            self.stage.parse_args(['--mode', mode, '--hdfs'])
        args = dict(self.default_args)
        #...some logic to set mode-related values...
        args['hdfs'] = True
        args['source'] = 'h'
        args['dest'] = 'h'
        self.check_args(args)
    if short:
         setattr(ProcessorStageArgsTestCase,
                 'test_override_hdfs_m_%s' % mode)
     else:
         setattr(ProcessorStageArgsTestCase,
                 'test_override_hdfs_mode_%s' % mode)

<...>
for m in modes:
    add_override_hdfs_mode(m)
    add_override_hdfs_mode(m, True)

This comment has been minimized.

Sign in to view

Copy link
@Evildoor

Evildoor Nov 20, 2018

Author Contributor

Yes, it is.


def add_arg(arg, val, short=False):
Expand Down Expand Up @@ -153,10 +202,58 @@ def f(self):
setattr(ProcessorStageArgsTestCase, 'test_mode_%s' % (val), f)


def add_override_hdfs(arg, val, short=False):
def f(self):
if short:
self.stage.parse_args(['--hdfs', '-' + arg[0], val])
else:
self.stage.parse_args(['--hdfs', '--' + arg, val])
args = dict(self.default_args)
args['hdfs'] = True
args['source'] = 'h'
args['dest'] = 'h'

This comment has been minimized.

Sign in to view

Copy link
@mgolosova

mgolosova Nov 20, 2018

Collaborator 
args[arg] = val

?

This comment has been minimized.

Sign in to view

Copy link
@Evildoor

Evildoor Nov 20, 2018

Author Contributor

What's the point? arg is either source or dest, hdfs overrides them both.

This comment has been minimized.

Sign in to view

Copy link
@mgolosova

mgolosova Nov 20, 2018

Collaborator

One reading the code (like me) knows nothing about this restriction, right? It should be at least specified in the comment, better -- checked explicitly, even better -- the function should work properly with any arg (I mean, at least with any arg that cause no additional changes in the ARGS values, like --mode would do).

This comment has been minimized.

Sign in to view

Copy link
@Evildoor

Evildoor Nov 21, 2018 •
edited
Loading

Author Contributor

I can see your point, I'll add a comment. "Better" options will have to wait because:

  1. Simply adding args[arg] = val will work, even with other arguments besides dest and source, but doing so means that the reader knows that --hdfs overrides everything.
  2. Proper way of handling it (and adapting to possible changes) is to move the rules about "argument A overrides argument B" into another hash (again, preferably inside pyDKB, not here) and make a single function which makes tests for arguments' interaction basing on this hash. It's possible, but let's leave that for later.

Update: ah, we already have a comment regarding overriding order. Ok then, will do the first option.

This comment has been minimized.

Sign in to view

Copy link
@mgolosova

mgolosova Nov 21, 2018

Collaborator

we already have a comment regarding overriding order. Ok then, will do the first option.

"The first option" was about args[arg] = val, right?

move the rules about "argument A overrides argument B" into another hash (again, preferably inside pyDKB, not here) and make a single function which makes tests for arguments' interaction basing on this hash.

Not sure that testing pyDKB basing on the pyDKB internals is a good idea. If I change this internal hash, test will still be passable even if my change breaks the logic that is described in help/usage message -- while it is exactly what it should check.
If the logic was changed, the one who did it must be notified (to avoid unintended changes).

This comment has been minimized.

Sign in to view

Copy link
@Evildoor

Evildoor Nov 22, 2018

Author Contributor

we already have a comment regarding overriding order. Ok then, will do the first option.

"The first option" was about args[arg] = val, right?

Yes.

move the rules about "argument A overrides argument B" into another hash (again, preferably inside pyDKB, not here) and make a single function which makes tests for arguments' interaction basing on this hash.

Not sure that testing pyDKB basing on the pyDKB internals is a good idea. If I change this internal hash, test will still be passable even if my change breaks the logic that is described in help/usage message -- while it is exactly what it should check.
If the logic was changed, the one who did it must be notified (to avoid unintended changes).

I see. Maybe I'll have something to say regarding this, but that's for some other time.
self.check_args(args)
if short:
setattr(ProcessorStageArgsTestCase,
'test_override_hdfs_%s_%s' % (arg[0], val), f)
else:
setattr(ProcessorStageArgsTestCase,
'test_override_hdfs_%s_%s' % (arg, val), f)


def add_override_mode(arg, val, mode_val, short=False):
def f(self):
if short:
self.stage.parse_args(['-' + arg[0], val, '--mode', mode_val])
else:
self.stage.parse_args(['--' + arg, val, '--mode', mode_val])
args = dict(self.default_args)
args['mode'] = mode_val
args['source'] = modes[mode_val][0]
args['dest'] = modes[mode_val][1]
args['eom'] = modes[mode_val][2]

This comment has been minimized.

Sign in to view

Copy link
@mgolosova

mgolosova Nov 20, 2018

Collaborator

It is what I mentioned in on of the previous comments (about reusing the modes hash).
By the way -- args['mode'] can also be pre-defined in modes, to ensure no one will forget it to specify (I could, easily).

This comment has been minimized.

Sign in to view

Copy link
@Evildoor

Evildoor Nov 20, 2018

Author Contributor

True, will do.
args['eop'] = modes[mode_val][3]
args[arg] = val
self.check_args(args)
if short:
setattr(ProcessorStageArgsTestCase,
'test_override_%s_%s_mode_%s' % (arg[0], val, mode_val), f)
else:
setattr(ProcessorStageArgsTestCase,
'test_override_%s_%s_mode_%s' % (arg, val, mode_val), f)


for a in args_to_add:
for v in args_to_add[a]:
add_arg(a, v)
add_arg(a, v, True)
add_override_hdfs(a, v)
add_override_hdfs(a, v, True)
for m in ('s', 'f', 'm'):

This comment has been minimized.

Sign in to view

Copy link
@mgolosova

mgolosova Nov 20, 2018

Collaborator

Not for m in modes, as below:

dkb/Utils/Dataflow/pyDKB/dataflow/stage/tests/test_ProcessorStage.py

Lines 257 to 259 in 088e180

for m in modes:
add_mode(m)
add_mode(m, True)

?

This comment has been minimized.

Sign in to view

Copy link
@Evildoor

Evildoor Nov 20, 2018

Author Contributor

I forgot to change this part, thank you for pointing out.
add_override_mode(a, v, m)
add_override_mode(a, v, m, True)


for m in modes:
add_mode(m)
add_mode(m, True)
Expand Down

0 comments on commit 088e180

Please sign in to comment.