Externally Executable Tool Tests 2 (with better API) by jmchilton · Pull Request #5628 · galaxyproject/galaxy

jmchilton · 2018-03-01T15:58:21Z

This is the same as #5545 but with the addition of 3513930. This added commit significantly improves the quality of the JSON exported from Galaxy API to describe tool tests (and by extension cleans up the internals of Galaxy's test parsing as well).

nsoranzo · 2018-03-01T16:39:54Z

+        repository_dir = self._repository_dir
+        if repository_dir:
+            for root, dirs, files in os.walk(repository_dir):
+                if '.' in dirs:


if '.hg' in dirs:

Huh - this was copied and pasted but doesn't make a lot of sense.

- for root, dirs, files in os.walk(os.path.join(tool_path, installed_tool_path)): - if '.' in dirs: - dirs.remove('.hg')

I think you must be correct about the original intent. I'll update with that fix.

nsoranzo · 2018-03-02T11:51:44Z

+        slept = 0
+        walltime_exceeded = kwd.get("maxseconds", None)
+        if walltime_exceeded is None:
+            walltime_exceeded = DEFAULT_TOOL_TEST_WAIT


Can we replace the previous 3 lines with:

walltime_exceeded = kwd.get("maxseconds", DEFAULT_TOOL_TEST_WAIT)

?

I can do this but I need to make sure ToolTestDescription doesn't allow None as a value for this. I'll make that change.

nsoranzo · 2018-03-02T11:57:21Z

+                sleep_amount *= 2
+            else:
+                exceeded = False
+                break


If you change break to return, you can get rid of exceeded.

nsoranzo · 2018-03-02T12:24:08Z

+            "test_index": test_index,
+            "inputs": {},
+            "error": True,
+            "exception": str(e),


Why not directly save the e object? That would simplify lines 710-713 in lib/galaxy/tools/verify/interactor.py

It was an object before but the result is JSONified and sent over the wire now - at least through certain paths of the code. This needs to be a str here - if you want I can simplify the other lines to always assume it is a string - I'm not sure if the object path through the code is still valid.

AFAIU, it seems ToolTestDescription.to_dict() does not process the exception attribute.

Good catch - but that is bug it really needs to I think or there is not reason to record it. I'll fix that.

nsoranzo · 2018-03-02T12:33:02Z

@@ -476,19 +475,6 @@ def cleanup_directory(tempdir):
 def setup_shed_tools_for_test(app, tmpdir, testing_migrated_tools, testing_installed_tools):
    """Modify Galaxy app's toolbox for migrated or installed tool tests."""
    # Store a jsonified dictionary of tool_id : GALAXY_TEST_FILE_DIR pairs.


Is this comment still relevant here?

No - probably not - I'll axe that.

nsoranzo · 2018-03-02T12:40:28Z

        self.dataset_collections_service = DatasetCollectionManager(self)
+        self.history_manager = HistoryManager(self)
+        self.dependency_resolvers_view = DependencyResolversView(self)
+        self.test_data_resolver = test_data.TestDataResolver(file_dirs=getattr(self.config, "tool_test_data_directories", None))


Why getattr(self.config, "tool_test_data_directories", None) instead of the simpler self.config.tool_test_data_directories ?

I do that frequently - the tool class for instance is used by the tool shed app I believe which doesn't have that attribute in the config. I think it is best to not assume as little about config as necessary to get a Tool that can be used as widely as possible.

But that's the Galaxy UniverseApplication, not Tool class, so this attribute should always be defined, right?

Indeed, missed that. I will change this for you.

nsoranzo · 2018-03-05T15:31:39Z

+            else:
+                return
+
+        if exceeded:


You need to remove also this line now that exceeded is gone.

mvdbeek · 2018-03-06T08:57:21Z

+DEFAULT_TOOL_TEST_WAIT = os.environ.get("GALAXY_TEST_DEFAULT_WAIT", 86400)
+
+DEFAULT_FTYPE = 'auto'
+DEFAULT_DBKEY = 'hg17'


Can we make that '?' ? hg17 is usually not defined on regular galaxy instances, leading to

File "script.py", line 39, in main tool_id, galaxy_interactor, test_index=test_index, tool_version=tool_version, register_job_data=_register_job_data File "/Users/mvandenb/src/galaxy/lib/galaxy/tools/verify/interactor.py", line 632, in verify_tool stage_data_in_history(galaxy_interactor, tool_id, testdef.test_data(), test_history) File "/Users/mvandenb/src/galaxy/lib/galaxy/tools/verify/interactor.py", line 52, in stage_data_in_history upload_waits.append(galaxy_interactor.stage_data_async(test_data, history, tool_id)) File "/Users/mvandenb/src/galaxy/lib/galaxy/tools/verify/interactor.py", line 279, in stage_data_async raise Exception("Request to upload dataset failed [%s]" % submit_response_object.content) Exception: Request to upload dataset failed [{"err_data": {"dbkey": "An invalid option was selected for dbkey, u'hg17', please verify."}, "err_msg": "An invalid option was selected for dbkey, u'hg17', please verify.", "err_code": 0}]

Changing that to ? works just fine in my limited tests.

Well somewhere there is a legacy test or 7 that is going to fail now but this seems like a good time to fix this.

I guess if some test does break - we could update the code to hit the API key and fetch the dbkeys and use hg17 if it is there or else revert to the new default of '?'.

Sounds good to me!

mvdbeek · 2018-03-06T09:00:43Z

Very nice!
$ python script.py -k <api_key> -t minimap2 -j /dev/stdout|jq

{
  "tests": [
    {
      "data": {
        "inputs": {
          "reference_source|reference_source_selector": "history",
          "fastq_input|fastq_input1": {
            "src": "hda",
            "id": "cb30f97aa5666a57"
          },
          "reference_source|ref_file": {
            "src": "hda",
            "id": "fb1f8927541ed886"
          },
          "fastq_input|fastq_input_selector": "single",
          "analysis_type_selector": "sr"
        },
        "job": {
          "tool_id": "toolshed.g2.bx.psu.edu/repos/iuc/minimap2/minimap2/2.3",
          "update_time": "2018-03-06T08:58:42.445108",
          "stdout": "",
          "outputs": {
            "alignment_output": {
              "src": "hda",
              "id": "28b4834f28bbe266",
              "uuid": "0f7342b8-d72a-4c98-bdde-9011ff0aa915"
            }
          },
          "command_line": "ln -f -s '/Users/mvandenb/src/galaxy/database/files/001/dataset_1296.dat' reference.fa && minimap2 -a -x sr    -t ${GALAXY_SLOTS:-4} reference.fa '/Users/mvandenb/src/galaxy/database/files/001/dataset_1297.dat' | samtools sort -@${GALAXY_SLOTS:-2} -O BAM -o '/Users/mvandenb/src/galaxy/database/files/001/dataset_1298.dat'",
          "exit_code": 0,
          "inputs": {
            "ref_file": {
              "src": "hda",
              "id": "fb1f8927541ed886",
              "uuid": "af999c2f-ad38-4679-b94d-f8160d79ec02"
            },
            "fastq_input1": {
              "src": "hda",
              "id": "cb30f97aa5666a57",
              "uuid": "d5eaf6b9-7980-4368-9e3c-9298ec97dfda"
            }
          },
          "state": "ok",
          "create_time": "2018-03-06T08:58:38.873519",
          "stderr": "[M::mm_idx_gen::0.002*1.89] collected minimizers\n[M::mm_idx_gen::0.004*3.75] sorted minimizers\n[M::main::0.004*3.71] loaded/built the index for 1 target sequence(s)\n[M::mm_mapopt_update::0.004*3.71] mid_occ = 1000\n[M::mm_idx_stat] kmer size: 21; skip: 11; is_HPC: 0; #seq: 1\n[M::mm_idx_stat::0.004*3.61] distinct minimizers: 2777 (100.00% are singletons); average occurrences: 1.000; average spacing: 5.967\n[M::worker_pipeline::0.006*3.48] mapped 100 sequences\n[M::main] Version: 2.4-r562-dirty\n[M::main] CMD: minimap2 -a -x sr -t 8 reference.fa /Users/mvandenb/src/galaxy/database/files/001/dataset_1297.dat\n[M::main] Real time: 0.008 sec; CPU: 0.023 sec\n",
          "model_class": "Job",
          "job_metrics": [
            {
              "raw_value": "2.0000000",
              "title": "Job Runtime (Wall Clock)",
              "name": "runtime_seconds",
              "value": "2 seconds",
              "plugin": "core"
            },
            {
              "raw_value": "8.0000000",
              "title": "Cores Allocated",
              "name": "galaxy_slots",
              "value": "8",
              "plugin": "core"
            },
            {
              "raw_value": "1520326720.0000000",
              "title": "Job Start Time",
              "name": "start_epoch",
              "value": "2018-03-06 09:58:40",
              "plugin": "core"
            },
            {
              "raw_value": "1520326722.0000000",
              "title": "Job End Time",
              "name": "end_epoch",
              "value": "2018-03-06 09:58:42",
              "plugin": "core"
            }
          ],
          "params": {
            "alignment_options": "{\"A\": \"\", \"B\": \"\", \"E\": \"\", \"-O2\": \"\", \"O\": \"\", \"s\": \"\", \"u\": null, \"z\": \"\", \"E2\": \"\"}",
            "io_options": "{\"Q\": \"false\", \"cs\": null, \"output_format\": \"BAM\", \"K\": \"\", \"L\": \"false\"}",
            "dbkey": "\"?\"",
            "analysis_type_selector": "\"sr\"",
            "mapping_options": "{\"g\": \"\", \"G\": \"\", \"F\": \"\", \"f\": \"\", \"m\": \"\", \"n\": \"\", \"p\": \"\", \"r\": \"\", \"X\": \"false\", \"N\": \"\"}",
            "fastq_input": "{\"__current_case__\": 1, \"fastq_input_selector\": \"single\", \"fastq_input1\": {\"values\": [{\"src\": \"hda\", \"id\": 1880}]}}",
            "reference_source": "{\"ref_file\": {\"values\": [{\"src\": \"hda\", \"id\": 1879}]}, \"reference_source_selector\": \"history\", \"__current_case__\": 1}",
            "indexing_options": "{\"I\": \"\", \"k\": \"\", \"w\": \"\"}",
            "chromInfo": "\"/Users/mvandenb/src/tool-data/shared/ucsc/chrom/?.len\""
          },
          "external_id": "58330",
          "id": "9fea3536e8286cc5",
          "user_email": "xxxxxxxxxxxx@yyy.com"
        }
      },
      "id": "minimap2-0",
      "has_data": true
    }
  ],
  "version": "0.1"
}

@mvdbeek

This makes a lot more sense, since the target Galaxy may not be configured with this dbkey. Thanks to @mvdbeek for the catch (galaxyproject#5628 (review)). Restore the legacy behavior with "export GALAXY_TEST_DEFAULT_DBKEY=hg17".

nsoranzo · 2018-03-06T12:29:42Z


 DEFAULT_FTYPE = 'auto'
-DEFAULT_DBKEY = 'hg17'
+# This following defualt dbkey was traditionally hg17 before Galaxy 18.05,


s/defualt/default/

@mvdbeek

This makes a lot more sense, since the target Galaxy may not be configured with this dbkey. Thanks to @mvdbeek for the catch (galaxyproject#5628 (review)). Restore the legacy behavior with "export GALAXY_TEST_DEFAULT_DBKEY=hg17".

@mvdbeek

This makes a lot more sense, since the target Galaxy may not be configured with this dbkey. Thanks to @mvdbeek for the catch (galaxyproject#5628 (review)). Restore the legacy behavior with "export GALAXY_TEST_DEFAULT_DBKEY=hg17".

nsoranzo · 2018-03-06T17:41:45Z

There's one unit test that keeps failing.

jmchilton · 2018-03-06T18:09:34Z

@nsoranzo Yeah - I saw that too - I've looked at it at a bit. It is quite baffling because everything was green before I added that last commit related to dbkey and I've tried running it locally and it doesn't fail for me - either as part of the whole unit test or alone.

I was hoping some other PR would have it fail also but it really seems to just be this one. Maybe something to do with the test data resolver changes - I'll have to figure it out.

jmchilton · 2018-03-06T18:44:56Z

Actually an unrelated PR does exhibit this error on Travis https://travis-ci.org/galaxyproject/galaxy/jobs/349953355 and against 18.01 no less. Odd.

jmchilton · 2018-03-06T18:59:56Z

Well the problem is we don't have a fixed sniffer order for the new bam types I think.

mvdbeek · 2018-03-06T19:01:30Z

I think that's my fault, this was introduced with #5589.
Some files could be sniffed to bam_native or bam_input_qname_sorted. We should disable the sniffer for the latter.

Only aligners should produce this datatype. If an alignment is not explicitly coordinate or queryname sorted it should be BamNative. Also addresses galaxyproject#5628 (comment), though I think the sniff order should be dependent on the order with which datatypes are loaded.

Can be shared between tests and TDTs. A step toward not requiring one to load the tool to test the tool (important for external testing of tools).

Should enable greater reuse from outside the test framework (e.g. standalone scripts and Planemo).

Working toward making interactor stand-alone and independent of the test framework.

This gets us a lot closer to being able to construct a ToolTestBuilder from simple dictionaries generated from the API for instance or from any tool really.

Use this is in tool testing. Prevents needing to load / have tools in memory to run tool tests - should allow external tool testing.

Previously this was set in the test framework but that doesn't help running Galaxies.

…shed.

…ools.

@mvdbeek

Based on conversations with @mvdbeek on https://github.com/galaxyproject/galaxy/pull/5545/files.

Capture more information about the test.

Cleanup up internal representations used to bridge the tool parser and the tool testing modules to produce more sensible dictified representations when exported via the API. This is mostly replacing tuples (which get converted to JSON lists) to dicts but there are some other small cleanups.

Thanks to 🦅👀.

@mvdbeek

This makes a lot more sense, since the target Galaxy may not be configured with this dbkey. Thanks to @mvdbeek for the catch (galaxyproject#5628 (review)). Restore the legacy behavior with "export GALAXY_TEST_DEFAULT_DBKEY=hg17".

mvdbeek · 2018-03-07T09:52:17Z

    <sniffer type="galaxy.datatypes.binary:H5"/>
    <sniffer type="galaxy.datatypes.binary:Bam"/>
+    <sniffer type="galaxy.datatypes.binary:BamQuerynameSorted" />
+    <sniffer type="galaxy.datatypes.binary:BamInputSorted" />


Thanks for this, I missed that! Can you move galaxy.datatypes.binary:BamInputSorted below BamNative ? We don't want to actually sniff that one (or give the impression that it can be sniffed).

Ugh - this was my attempt to fix the failing test before you did. This commit doesn’t belong here - sorry about that and thanks for catching it.

Okay - I've stripped the commit. Thanks again for the catch.

mvdbeek · 2018-03-07T14:53:45Z

Exciting! Thanks a lot @jmchilton !

jmchilton · 2018-03-07T15:17:59Z

@mvdbeek Thank you and @nsoranzo for the detailed review and testing! I appreciate the merge.

I've created a list of follow up work with #5651

jmchilton added kind/enhancement area/testing area/API labels Mar 1, 2018

jmchilton force-pushed the external_tool_tests_better_api branch from 3513930 to 8b963eb Compare March 1, 2018 16:00

galaxybot added this to the 18.05 milestone Mar 1, 2018

nsoranzo reviewed Mar 1, 2018

View reviewed changes

jmchilton force-pushed the external_tool_tests_better_api branch from 8b963eb to c60a520 Compare March 1, 2018 17:40

nsoranzo reviewed Mar 2, 2018

View reviewed changes

nsoranzo reviewed Mar 5, 2018

View reviewed changes

jmchilton force-pushed the external_tool_tests_better_api branch from 9d017bb to 6ba55dc Compare March 5, 2018 15:58

nsoranzo approved these changes Mar 5, 2018

View reviewed changes

mvdbeek reviewed Mar 6, 2018

View reviewed changes

nsoranzo reviewed Mar 6, 2018

View reviewed changes

jmchilton force-pushed the external_tool_tests_better_api branch from 99b3bc9 to 948be85 Compare March 6, 2018 12:30

jmchilton force-pushed the external_tool_tests_better_api branch from 948be85 to ec399c3 Compare March 6, 2018 17:24

mvdbeek mentioned this pull request Mar 6, 2018

[18.01] Do not sniff BamInputSorted #5644

Merged

jmchilton added 4 commits March 6, 2018 20:07

Create fewer objects when instantiating tools.

58ed030

New abstraction to obtain tool test data paths.

5e6c1c9

Can be shared between tests and TDTs. A step toward not requiring one to load the tool to test the tool (important for external testing of tools).

Eliminate shed tools dict no longer used.

210d5cb

Eliminate tool shed test helper no longer used.

58ac3b6

jmchilton added 17 commits March 6, 2018 20:07

Refactor more tool test verification code into galaxy-lib.

7e372c5

Should enable greater reuse from outside the test framework (e.g. standalone scripts and Planemo).

Refactor polling code from testcase into more standalone interactor.

70e76ca

Working toward making interactor stand-alone and independent of the test framework.

Refactor much more tool testing code into galaxy-lib.

2899375

Refactor ToolTestBuilder to not require an in-memory Tool object.

508047f

This gets us a lot closer to being able to construct a ToolTestBuilder from simple dictionaries generated from the API for instance or from any tool really.

Serialize tool test descriptions through the API.

c3af6c8

Use this is in tool testing. Prevents needing to load / have tools in memory to run tool tests - should allow external tool testing.

Refactor tool test outline construction to allow more reuse.

b1779e1

Setup a default GALAXY_TEST_FILE_DIR in case unset in the Galaxy itself.

cd00261

Previously this was set in the test framework but that doesn't help running Galaxies.

Refactor data manager test to allow more integration tests with tool …

6f908a7

…shed.

Integration test case for running tool tests against shed installed t…

662f29f

…ools.

Revise repository directory calculation in tool class.

db6dafa

Based on conversations with @mvdbeek on https://github.com/galaxyproject/galaxy/pull/5545/files.

Better default configuration of Galaxy test data directory.

e7f7759

Add structured output json example to galaxy.tools.verify.script.

dc080b9

Capture more information about the test.

Stronger security around test data path resolution, now with tests.

52b6bf2

Bug fix for exclude mercurial repo from test data search.

28ba027

Thanks to 🦅👀.

Cleanup external tool testing based on @nsoranzo's comments.

227bf4a

Switch default tool test dbkey to '?'.

f741ab1

This makes a lot more sense, since the target Galaxy may not be configured with this dbkey. Thanks to @mvdbeek for the catch (galaxyproject#5628 (review)). Restore the legacy behavior with "export GALAXY_TEST_DEFAULT_DBKEY=hg17".

jmchilton force-pushed the external_tool_tests_better_api branch from ec399c3 to ffc929d Compare March 7, 2018 01:08

mvdbeek reviewed Mar 7, 2018

View reviewed changes

jmchilton force-pushed the external_tool_tests_better_api branch from ffc929d to f741ab1 Compare March 7, 2018 12:18

mvdbeek merged commit 3afb4ab into galaxyproject:dev Mar 7, 2018

jmchilton mentioned this pull request Mar 7, 2018

External Tool Test Follow Up Work #5651

Open

4 tasks

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Mar 8, 2018

Fix tool test job data collection broken in galaxyproject#5628.

c673a73

jmchilton mentioned this pull request Mar 8, 2018

Tool Testing Fix and Enhancements #5667

Merged

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Mar 9, 2018

Fix tool test job data collection broken in galaxyproject#5628.

f715ae5

nsoranzo deleted the external_tool_tests_better_api branch November 7, 2025 16:14

nsoranzo mentioned this pull request Nov 7, 2025

Add client helper for tool test cases galaxyproject/bioblend#522

Merged

Conversation

jmchilton commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvdbeek commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsoranzo commented Mar 6, 2018

Uh oh!

jmchilton commented Mar 6, 2018

Uh oh!

jmchilton commented Mar 6, 2018

Uh oh!

jmchilton commented Mar 6, 2018

Uh oh!

mvdbeek commented Mar 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvdbeek commented Mar 7, 2018

Uh oh!

jmchilton commented Mar 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mvdbeek commented Mar 6, 2018 •

edited

Loading