Skip to content

Commit

Permalink
Add New Tests, Fix Bug Uncovered From Test
Browse files Browse the repository at this point in the history
Added two new tests to test for `valid_config` not being messed with by
previous assignments to it.

This proved that the previous design of setting `valid_config` to the
value of `validate_destination` in `validate_config` caused errant
behaviour when a default destination was invalid but a tool default
destination was valid. This case caused the program to still end up
returning `valid_config` as True even when an invalid default destination
was found.
  • Loading branch information
Matthew Spelchak committed Mar 5, 2018
1 parent ead6dbf commit 920be90
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 11 deletions.
32 changes: 21 additions & 11 deletions lib/galaxy/jobs/dynamic_tool_destination.py
Original file line number Diff line number Diff line change
Expand Up @@ -851,11 +851,13 @@ def infinite_defaultdict():

if return_bool:
verbose = True

elif obj is not None and 'verbose' in obj and isinstance(obj['verbose'], bool):
verbose = obj['verbose']
else:
valid_config = False
if obj:
log.debug("Verbose value '" + str(obj['verbose']) + "' is not True or False! Falling back to verbose...")
verbose = True

if not return_bool and verbose:
log.debug("Running config validation...")
Expand All @@ -874,11 +876,13 @@ def infinite_defaultdict():
if 'default_destination' in obj:
suggestion = None
if isinstance(obj['default_destination'], str):
valid_config = validate_destination(app, obj['default_destination'],
dest_err_default_dest,
(obj['default_destination']))
if valid_config:
is_valid = validate_destination(app, obj['default_destination'],
dest_err_default_dest,
(obj['default_destination']))
if is_valid:
new_config["default_destination"] = obj['default_destination']
else:
valid_config = False

elif isinstance(obj['default_destination'], dict):

Expand All @@ -889,14 +893,16 @@ def infinite_defaultdict():
if isinstance(obj['default_destination']['priority'][priority],
str):
priority_list.add(priority)
valid_config = validate_destination(
is_valid = validate_destination(
app, obj['default_destination']['priority'][priority],
dest_err_default_dest,
(obj['default_destination']['priority'][priority]))

if valid_config:
if is_valid:
new_config["default_destination"]['priority'][priority] = (
obj['default_destination']['priority'][priority])
else:
valid_config = False
if len(priority_list) < 1:
error = ("No valid priorities found!")
if verbose:
Expand Down Expand Up @@ -1004,14 +1010,16 @@ def infinite_defaultdict():
if "default_destination" in curr:
suggestion = None
if isinstance(curr['default_destination'], str):
valid_config = validate_destination(app,
is_valid = validate_destination(app,
curr['default_destination'],
dest_err_tool_default_dest,
(tool, curr['default_destination']))
if valid_config:
if is_valid:
new_config['tools'][tool]['default_destination'] = (
(curr['default_destination']))
tool_has_default = True
else:
valid_config = False
elif isinstance(curr['default_destination'], dict):

if ('priority' in curr['default_destination']
Expand All @@ -1022,13 +1030,15 @@ def infinite_defaultdict():
if priority in priority_list:
if isinstance(destination, str):

valid_config = validate_destination(
is_valid = validate_destination(
app, destination,
dest_err_tool_default_dest,
(tool, curr['default_destination']['priority'][priority]))
if valid_config:
if is_valid:
new_config['tools'][tool]['default_destination']['priority'][priority] = destination
tool_has_default = True
else:
valid_config = False

else:
error = ("No default '" + str(priority)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,30 @@ def test_typo_in_case(self, l):
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "Default destination 'destinationf' does not appear in the job configuration. Did you mean 'DestinationF'?"),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Finished config validation.')
)

@log_capture()
def test_invalid_verbose_value(self, l):
dt.parse_yaml(path=yt.ivYMLTest171, job_conf_path=job_conf_path, test=True)
l.check(
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "Verbose value 'notavalue' is not True or False! Falling back to verbose..."),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Running config validation...'),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Finished config validation.')
)

@log_capture()
def test_invalid_default_dest_valid_tool_default_dest_bool(self, l):
self.assertFalse(dt.parse_yaml(path=yt.ivYMLTest172, job_conf_path=job_conf_path, test=True, return_bool=True))
l.check(
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "Default destination 'fake_destination' does not appear in the job configuration."),
)

@log_capture()
def test_valid_default_dest_invalid_tool_default_dest_bool(self, l):
self.assertFalse(dt.parse_yaml(path=yt.ivYMLTest173, job_conf_path=job_conf_path, test=True, return_bool=True))
l.check(
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "Default destination for 'blah': 'fake_destination' does not appear in the job configuration."),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "Tool 'blah' does not have rules nor a default_destination!"),
)

# ================================Valid yaml files==============================
@log_capture()
Expand Down
27 changes: 27 additions & 0 deletions test/unit/jobs/dynamic_tool_destination/ymltests.py
Original file line number Diff line number Diff line change
Expand Up @@ -1337,3 +1337,30 @@
default_priority: med
verbose: True
'''

# Invalid verbose setting
ivYMLTest171 = '''
default_destination:
priority:
med: DestinationF
default_priority: med
verbose: notavalue
'''

#invalid default destination and valid tool default destination
ivYMLTest172 = '''
default_destination: fake_destination
tools:
blah:
default_destination: cluster_default
verbose: True
'''

#valid default destination and invalid tool default destination
ivYMLTest173 = '''
default_destination: cluster_default
tools:
blah:
default_destination: fake_destination
verbose: True
'''

0 comments on commit 920be90

Please sign in to comment.