Scottx611x/tool manager updates (#1805)

* Generic Selenium testing base class & tool_manager tests refactoring

* Update isort.cfg to include selenium_testing

* Simplify view

* Simplify ToolDefinition generation

* Add more test coverage & cleanup

* Add `annotation` field to ToolDefinition & `tool_launch_configuration` to Tool

* Fix tests and add more test coverage

* Add trailing slash

.isort.cfg

@@ -11,6 +11,7 @@ known_first_party =
     file_server,
     file_store,
     galaxy_connector,
+    selenium_testing,
     tool_manager,
     user_files_manager,
     visualization_manager,

refinery/selenium_testing/tests.py

@@ -1,35 +1,24 @@
 from django.contrib.auth.models import User
-from django.contrib.staticfiles.testing import StaticLiveServerTestCase
-
-from pyvirtualdisplay import Display
-from selenium import webdriver
 
 from core.management.commands.create_public_group import create_public_group
 from core.management.commands.create_user import init_user
 from core.models import Analysis, DataSet
-
 from factory_boy.utils import make_analyses_with_single_dataset, make_datasets
-from selenium_testing.utils import (
-    assert_body_text, assert_text_within_id, delete_from_ui, login,
-    MAX_WAIT, wait_until_class_visible, wait_until_id_clickable,
-    wait_until_id_visible
-)
-
 
-class SeleniumTestBase(StaticLiveServerTestCase):
-    """Abstract base class to be used for all Selenium-based tests."""
+from .utils import (MAX_WAIT, SeleniumTestBaseGeneric, assert_body_text,
+                    assert_text_within_id, delete_from_ui, login,
+                    wait_until_class_visible, wait_until_id_clickable,
+                    wait_until_id_visible)
 
-    # Don't delete data migration data after test runs: http://bit.ly/2lAYqVJ
-    serialized_rollback = True
 
+class RefinerySeleniumTestBase(SeleniumTestBaseGeneric):
+    """
+    Base class for selenium tests specifically testing Refinery UI components
+    """
     def setUp(self, site_login=True, initialize_guest=True,
               public_group_needed=True):
 
-        # Start a pyvirtualdisplay for geckodriver to interact with
-        self.display = Display(visible=0, size=(1366, 768))
-        self.display.start()
-        self.browser = webdriver.Firefox()
-        self.browser.maximize_window()
+        super(RefinerySeleniumTestBase, self).setUp()
 
         if initialize_guest:
             init_user("guest", "guest", "guest@coffee.com", "Guest", "Guest",
@@ -42,21 +31,15 @@ def setUp(self, site_login=True, initialize_guest=True,
         if public_group_needed:
             create_public_group()
 
-    def tearDown(self):
-        # NOTE: quit() destroys ANY currently running webdriver instances.
-        # This could become an issue if tests are ever run in parallel.
-        self.browser.quit()
-        self.display.stop()
-
 
-class NoLoginTestCase(SeleniumTestBase):
+class NoLoginTestCase(RefinerySeleniumTestBase):
     """
     Ensure that Refinery looks like it should when there is no currently
     logged in user
     """
 
-    # SeleniumTestBase.setUp(): We don't need to login or initialize the
-    # guest user this time
+    # RefinerySeleniumTestBase.setUp(): We don't need to login or
+    # initialize the guest user this time
     def setUp(self, site_login=True, initialize_guest=True,
               public_group_needed=False):
         super(NoLoginTestCase, self).setUp(initialize_guest=False,
@@ -92,7 +75,7 @@ def test_login_not_required(self):
         # TODO: All sections are empty right now
 
 
-class DataSetsPanelTestCase(SeleniumTestBase):
+class DataSetsPanelTestCase(RefinerySeleniumTestBase):
     """
     Ensure that the DataSet upload button and DataSet Preview look like
     they're behaving normally
@@ -133,7 +116,7 @@ def test_upload_button(self):
         )
 
 
-class UiDeletionTestCase(SeleniumTestBase):
+class UiDeletionTestCase(RefinerySeleniumTestBase):
     """Ensure proper deletion of DataSets and Analyses from the UI"""
 
     def test_dataset_deletion(self, total_datasets=2):

refinery/selenium_testing/utils.py

@@ -1,13 +1,35 @@
+from django.contrib.staticfiles.testing import StaticLiveServerTestCase
+from pyvirtualdisplay import Display
+from selenium import webdriver
+from selenium.common.exceptions import TimeoutException
 from selenium.webdriver.common.by import By
-from selenium.webdriver.support.ui import WebDriverWait
 from selenium.webdriver.support import expected_conditions as ec
-from selenium.common.exceptions import TimeoutException
+from selenium.webdriver.support.ui import WebDriverWait
 
 # The maximum amount of time that we allow an ExpectedCondition to wait
 # before timing out.
 MAX_WAIT = 60
 
 
+class SeleniumTestBaseGeneric(StaticLiveServerTestCase):
+    """Base class to be used for all selenium-based tests"""
+
+    # Don't delete data migration data after test runs: http://bit.ly/2lAYqVJ
+    serialized_rollback = True
+
+    def setUp(self):
+        self.display = Display(visible=0, size=(1366, 768))
+        self.display.start()
+        self.browser = webdriver.Firefox()
+        self.browser.maximize_window()
+
+    def tearDown(self):
+        # NOTE: quit() destroys ANY currently running webdriver instances.
+        # This could become an issue if tests are ever run in parallel.
+        self.browser.quit()
+        self.display.stop()
+
+
 def login(selenium, live_server_url):
     """
     Helper method to login to the StaticLiveServerTestCase Refinery instance

refinery/tool_manager/migrations/0018_auto_20170615_1629.py

@@ -0,0 +1,33 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('tool_manager', '0017_tooldefinition_extra_directories'),
+    ]
+
+    operations = [
+        migrations.RenameField(
+            model_name='tool',
+            old_name='file_relationships',
+            new_name='tool_launch_configuration',
+        ),
+        migrations.RemoveField(
+            model_name='tool',
+            name='parameters',
+        ),
+        migrations.RemoveField(
+            model_name='tooldefinition',
+            name='extra_directories',
+        ),
+        migrations.AddField(
+            model_name='tooldefinition',
+            name='annotation',
+            field=models.TextField(default='{}'),
+            preserve_default=False,
+        ),
+    ]

refinery/tool_manager/models.py

@@ -1,4 +1,5 @@
 import ast
+import json
 import logging
 import re
 
@@ -8,11 +9,9 @@
 from django.dispatch import receiver
 from django.http import (HttpResponseBadRequest, HttpResponseServerError,
                          JsonResponse)
-
-from django_extensions.db.fields import UUIDField
 from django_docker_engine.docker_utils import (DockerClientWrapper,
                                                DockerContainerSpec)
-
+from django_extensions.db.fields import UUIDField
 from docker.errors import APIError
 
 from core.models import Analysis, OwnableResource, WorkflowEngine
@@ -161,10 +160,7 @@ class ToolDefinition(models.Model):
         max_length=500,
         blank=True
     )
-    extra_directories = models.CharField(
-        max_length=500,
-        blank=True
-    )
+    annotation = models.TextField()
     galaxy_workflow_id = models.CharField(
         max_length=250,
         blank=True
@@ -174,6 +170,20 @@ class ToolDefinition(models.Model):
     def __str__(self):
         return "{}: {} {}".format(self.tool_type, self.name, self.uuid)
 
+    def get_annotation(self):
+        return json.loads(self.annotation)
+
+    def get_extra_directories(self):
+        if self.tool_type == ToolDefinition.VISUALIZATION:
+            try:
+                return self.get_annotation()["extra_directories"]
+            except KeyError:
+                logger.error("ToolDefinition: %s's annotation is missing its "
+                             "`extra_directories` key.", self.name)
+                raise
+        else:
+            raise NotImplementedError
+
 
 @receiver(pre_delete, sender=ToolDefinition)
 def delete_parameters_and_output_files(sender, instance, *args, **kwargs):
@@ -228,8 +238,7 @@ class Tool(OwnableResource):
         unique=True,
         blank=True
     )
-    file_relationships = models.TextField()
-    parameters = models.TextField()
+    tool_launch_configuration = models.TextField()
     tool_definition = models.ForeignKey(ToolDefinition)
 
     class Meta:
@@ -254,13 +263,9 @@ def launch(self):
                 container_input_path=(
                     self.tool_definition.container_input_path
                 ),
-                input={
-                    "file_relationships": ast.literal_eval(
-                        self.file_relationships
-                    )
-                },
-                extra_directories=ast.literal_eval(
-                    self.tool_definition.extra_directories
+                input={"file_relationships": self.get_file_relationships()},
+                extra_directories=(
+                    self.tool_definition.get_extra_directories()
                 )
             )
             try:
@@ -288,31 +293,46 @@ def get_relative_container_url(self):
             self.container_name
         )
 
+    def get_tool_launch_config(self):
+        return json.loads(self.tool_launch_configuration)
+
+    def get_file_relationships(self):
+        return ast.literal_eval(
+            self.get_tool_launch_config()["file_relationships"]
+        )
+
     def get_tool_name(self):
         return self.tool_definition.name
 
     def get_tool_type(self):
         return self.tool_definition.tool_type
 
+    def set_tool_launch_config(self, tool_launch_config):
+        self.tool_launch_configuration = json.dumps(tool_launch_config)
+        self.save()
+
     def update_file_relationships_string(self):
         """
         Replace a Tool's Node uuids in its `file_relationships` string with
         their respective FileStoreItem's urls. No error handling here since
         this method is only called in an atomic transaction.
         """
+
+        tool_launch_config = self.get_tool_launch_config()
+
         node_uuids = re.findall(
             r'[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}',
-            self.file_relationships
+            tool_launch_config["file_relationships"]
         )
 
         for uuid in node_uuids:
             file_url = get_file_url_from_node_uuid(uuid)
-            self.file_relationships = self.file_relationships.replace(
-                uuid,
-                "'{}'".format(file_url)
+            tool_launch_config["file_relationships"] = (
+                tool_launch_config["file_relationships"].replace(
+                    uuid, "'{}'".format(file_url)
+                )
             )
-
-        self.save()
+        self.set_tool_launch_config(tool_launch_config)
 
 
 @receiver(pre_delete, sender=Tool)

refinery/tool_manager/test_data/workflows/LIST:PAIR:LIST.json

@@ -0,0 +1,61 @@
+{ "name": "Test LIST:PAIR:LIST",
+  "tool_type": "WORKFLOW",
+  "galaxy_workflow_id": "66b2fe95-9250-425d-86ae-5953c1d6e5b6",
+  "annotation": {
+    "refinery_type": "analysis",
+    "description": "This workflow has a nested LIST of PAIRs of LISTs",
+    "parameters": [
+       {
+        "name": "Integer Param",
+        "description": "Integer Param description",
+        "value_type": "INTEGER",
+        "default_value": 1337,
+        "galaxy_workflow_step": 1
+      },
+      {
+        "name": "Attribute Param",
+        "description": "Attribute Param description",
+        "value_type": "ATTRIBUTE",
+        "default_value": "Species",
+        "galaxy_workflow_step": 4
+      },
+      {
+        "name": "File Param",
+        "description": "File Param description",
+        "value_type": "FILE",
+        "default_value": "/media/file_store/file.cool",
+        "galaxy_workflow_step": 5
+      }
+    ],
+    "output_files": [
+      {
+        "filetype": {"name": "FASTQ"},
+        "name": "Cool Input File",
+        "description": "Cool Input File Description"
+      }
+    ],
+    "file_relationship": {
+      "value_type": "LIST",
+      "name": "List of Pairs",
+      "file_relationship": {
+        "value_type": "PAIR",
+        "name": "Pair of Lists",
+        "file_relationship": {
+          "file_relationship": {},
+          "value_type": "LIST",
+          "name": "Lists",
+          "input_files": [
+            {
+              "allowed_filetypes": [
+                {"name": "FASTQ"},
+                {"name": "BAM"}
+              ],
+              "name": "Cool Input Files",
+              "description": "Cool Input Files Description"
+            }
+          ]
+        }
+      }
+    }
+  }
+}