Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Scottx611x/configurable branch for vis tool registry #2942

Merged
merged 6 commits into from
Aug 15, 2018
Merged

Scottx611x/configurable branch for vis tool registry #2942

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8c82d4f
Add option to specify a Vis Tool registry branch for `load_tools`
scottx611x Aug 9, 2018
281b76b
Add test coverage
scottx611x Aug 9, 2018
a3e0a57
Add mutually exclusive groups and argument groups to avoid doing our …
scottx611x Aug 15, 2018
460eaf9
Update tests to conform to new argument group specs
scottx611x Aug 15, 2018
a16d7ec
Merge remote-tracking branch 'origin/develop' into scottx611x/configu…
scottx611x Aug 15, 2018
28805f9
Remove assertion in non test method
scottx611x Aug 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 18 additions & 9 deletions refinery/tool_manager/management/commands/load_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,34 @@ class Command(BaseCommand):
"or generates workflow tool definitions"

def add_arguments(self, parser):
parser.add_argument(
mutually_exclusive_group = parser.add_mutually_exclusive_group(
required=True
)
visualization_group = parser.add_argument_group(
'Refinery VisualizationTools'
)
mutually_exclusive_group.add_argument(
'--visualizations',
nargs='+',
dest='visualizations',
help='Generate ToolDefinitions for visualizations, '
'either by filename, or from the registry'
'either by filename, or from the registry',
metavar='<local path to annotation json or name from registry>'
)
parser.add_argument(
mutually_exclusive_group.add_argument(
'--workflows',
action='store_true',
dest='workflows',
help='Generate ToolDefinitions for properly annotated '
'Galaxy-based Workflows'
)
visualization_group.add_argument(
'--branch',
action='store',
default='master',
dest='branch',
help='Branch from the registry to target for Vis tool installation'
)
parser.add_argument(
'--force',
action='store_true',
Expand All @@ -45,7 +59,6 @@ def add_arguments(self, parser):
def __init__(self):
super(Command, self).__init__()
self.force = False
self.visualization_registry_branch = "master"
self.raw_registry_url = \
settings.REFINERY_VISUALIZATION_REGISTRY.replace(
"github",
Expand All @@ -68,18 +81,14 @@ def handle(self, *args, **options):
)
visualizations = options["visualizations"]
is_workflow_mode = options["workflows"]
if not (visualizations or is_workflow_mode):
raise CommandError(
'Either --workflows or --visualizations is required'
)

self.force = options["force"]

if self.force:
self._confirmation_loop()
if is_workflow_mode:
self._generate_workflows()
if visualizations:
self.visualization_registry_branch = options["branch"]
self._load_visualization_definitions(visualizations)

@staticmethod
Expand Down
60 changes: 37 additions & 23 deletions refinery/tool_manager/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,12 +270,15 @@ def setUp(self):
self.django_docker_cleanup_wait_time
)

@mock.patch("django_docker_engine.docker_utils.DockerClientWrapper.pull")
def load_visualizations(
self,
docker_pull_mock,
visualizations=["{}/visualizations/igv.json".format(TEST_DATA_PATH)]
):
# TODO: More mocking, so Docker image is not downloaded
call_command("load_tools", visualizations=visualizations)
call_command("load_tools", "--visualizations",
" ".join(visualizations))
self.assertTrue(docker_pull_mock.called)
return visualizations

def tearDown(self):
Expand Down Expand Up @@ -1151,7 +1154,7 @@ def test_load_tools_management_command(self):
CommandError,
call_command,
"load_tools",
workflows=True
"--workflows"
)

self.assertEqual(ToolDefinition.objects.count(), 0)
Expand All @@ -1172,11 +1175,8 @@ def test_load_tools_overwrites_visualizations_if_forced(
original_ids = [t.id for t in ToolDefinition.objects.all()]

# Create new VisualizationToolDefinition with --force
call_command(
"load_tools",
visualizations=visualizations,
force=True
)
call_command("load_tools", "--visualizations",
" ".join(visualizations), "--force")
new_ids = [t.id for t in ToolDefinition.objects.all()]

# Assert that the new visualization tool definitions id's were
Expand All @@ -1192,15 +1192,11 @@ def test_load_tools_overwrites_workflows_if_forced(
return_value={self.workflow_engine.uuid: self.valid_workflows}
) as get_wf_mock:
# Create WorkflowToolDefinition
call_command("load_tools", workflows=True)
call_command("load_tools", "--workflows")
original_ids = [t.id for t in ToolDefinition.objects.all()]

# Create new WorkflowToolDefinition with --force
call_command(
"load_tools",
workflows=True,
force=True
)
call_command("load_tools", "--workflows", "--force")
new_ids = [t.id for t in ToolDefinition.objects.all()]

# Assert that the new workflow tool definitions id's were
Expand All @@ -1219,11 +1215,7 @@ def test_load_tools_with_force_allows_user_dismissal(
):
with self.assertRaises(SystemExit):
# Create WorkflowToolDefinition
call_command(
"load_tools",
workflows=True,
force=True
)
call_command("load_tools", "--workflows", "--force")

self.assertEqual(ToolDefinition.objects.count(), 0)

Expand All @@ -1235,7 +1227,7 @@ def test_load_tools_command_error_if_get_workflows_fails(
side_effect=RuntimeError
):
with self.assertRaises(CommandError):
call_command("load_tools", workflows=True)
call_command("load_tools", "--workflows")

def test_load_tools_multiple_times_skips_without_deletion(
self
Expand All @@ -1244,10 +1236,10 @@ def test_load_tools_multiple_times_skips_without_deletion(
self.mock_get_workflows_reference,
return_value={self.workflow_engine.uuid: self.valid_workflows}
):
call_command("load_tools", workflows=True)
call_command("load_tools", "--workflows")
tool_definitions_a = [t for t in ToolDefinition.objects.all()]

call_command("load_tools", workflows=True)
call_command("load_tools", "--workflows")
tool_definitions_b = [t for t in ToolDefinition.objects.all()]

self.assertEqual(tool_definitions_a, tool_definitions_b)
Expand Down Expand Up @@ -1281,6 +1273,27 @@ def test_load_tools_error_message_yields_vis_registry_info(
context.exception.message
)

def test_load_tools_command_error_if_branch_and_workflows_specified(self):
with self.assertRaises(CommandError):
call_command("load_tools", "--workflows",
branch="fake-branch-name")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in a similar vein, if we were using more of the built-in machinery, I don't think we'd need explicit checks on their error handling.


@mock.patch.object(LoadToolsCommand, "_load_visualization_definitions")
def test_load_tools_command_registry_branch_specified(
self, load_vis_mock
):
visualization_tool_registry_branch = "fake-branch-name"
command = LoadToolsCommand()
command.handle(
workflows=False,
visualizations=["test-vis-tool-name"],
branch=visualization_tool_registry_branch,
force=False
)
self.assertEqual(command.visualization_registry_branch,
visualization_tool_registry_branch)
self.assertTrue(load_vis_mock.called)

def test_workflow_pair_too_many_inputs(self):
with open(
"{}/workflows/PAIR_too_many_inputs.json".format(TEST_DATA_PATH)
Expand Down Expand Up @@ -1364,7 +1377,8 @@ def _assert_visualization_tool_def_exception_contents(
)
]
with self.assertRaises(exception) as context:
call_command("load_tools", visualizations=visualizations)
call_command("load_tools", "--visualizations",
" ".join(visualizations))
[self.assertIn(message, context.exception.message)
for message in messages]

Expand Down