More concise, readable tool execution testing.

[jmchilton]

I keep adding these really verbose tool tests that are very tied to the current API and very low level. I need a version that can be remapped to the tool request API in bulk and I'd love to have a version that was a little more robust and readable. This PR introduces some infrastructure to do that and migrates a bunch of the older tests that test "tool execution behavior" to the new paradigm.

A simple example

before

A class method:

class TestToolsAPI(...):
     ...

    @skip_without_tool("expression_forty_two")
    def test_galaxy_expression_tool_simplest(self):
        history_id = self.dataset_populator.new_history()
        run_response = self._run("expression_forty_two", history_id)
        self._assert_status_code_is(run_response, 200)
        self.dataset_populator.wait_for_history(history_id, assert_ok=True)
        output_content = self.dataset_populator.get_history_dataset_content(history_id)
        assert output_content == "42"

after

A simple function heavily leveraging pytest fixtures:

@requires_tool_id("expression_forty_two")
def test_galaxy_expression_tool_simplest(required_tool: RequiredTool):
    required_tool.execute.assert_has_single_job.with_single_output.with_contents("42")

We're describing the execution and tests against it in many fewer characters and in properties that read a lot like English so I think it should make these tests more readable. Parts of this may be "too clever" - I'm not a huge fan of plumbing the depths of Pytest features - but injecting an object based on the decorated tool ID has already caught a bug where they didn't match and leads to nice de-duplication of the tool ID string. We essentially do no testing of the @skip_without_tool decorator in the current version of all of this testing. And breaking the methods out the test class does make reasoning about the fixutres a little cleaner I think and simplifies calling these tests from the CLI.

A few more complicated examples:

before

    @skip_without_tool("identifier_in_conditional")
    def test_identifier_map_over_multiple_input_in_conditional(self, history_id):
        hdca_id = self._build_pair(history_id, ["123", "456"])
        inputs = {
            "outer_cond|input1": {"src": "hdca", "id": hdca_id},
        }
        create_response = self._run("identifier_in_conditional", history_id, inputs)
        self._assert_status_code_is(create_response, 200)
        create = create_response.json()
        outputs = create["outputs"]
        jobs = create["jobs"]
        implicit_collections = create["implicit_collections"]
        assert len(jobs) == 1
        assert len(outputs) == 1
        assert len(implicit_collections) == 0
        output1 = outputs[0]
        output1_content = self.dataset_populator.get_history_dataset_content(history_id, dataset=output1)
        assert output1_content.strip() == "forward\nreverse"

after

@requires_tool_id("identifier_in_conditional")
def test_identifier_map_over_multiple_input_in_conditional_legacy_format(
    target_history: TargetHistory, required_tool: RequiredTool
):
    hdca = target_history.with_pair(["123", "456"])
    execute = required_tool.execute.with_inputs(
        {
            "outer_cond|input1": hdca.src_dict,
        }
    )
    execute.assert_has_single_job.assert_has_single_output.with_contents_stripped("forward\nreverse")

before

    @skip_without_tool("multi_data_param")
    def test_multidata_param(self):
        with self.dataset_populator.test_history(require_new=False) as history_id:
            hda1 = dataset_to_param(self.dataset_populator.new_dataset(history_id, content="1\t2\t3"))
            hda2 = dataset_to_param(self.dataset_populator.new_dataset(history_id, content="4\t5\t6"))
            inputs = {
                "f1": {"batch": False, "values": [hda1, hda2]},
                "f2": {"batch": False, "values": [hda2, hda1]},
            }
            response = self._run("multi_data_param", history_id, inputs, assert_ok=True)
            output1 = response["outputs"][0]
            output2 = response["outputs"][1]
            output1_content = self.dataset_populator.get_history_dataset_content(history_id, dataset=output1)
            output2_content = self.dataset_populator.get_history_dataset_content(history_id, dataset=output2)
            assert output1_content == "1\t2\t3\n4\t5\t6\n", output1_content
            assert output2_content == "4\t5\t6\n1\t2\t3\n", output2_content

after

@requires_tool_id("multi_data_param")
def test_multidata_param(target_history: TargetHistory, required_tool: RequiredTool):
    hda1 = target_history.with_dataset("1\t2\t3").src_dict
    hda2 = target_history.with_dataset("4\t5\t6").src_dict
    execution = required_tool.execute.with_inputs(
        {
            "f1": {"batch": False, "values": [hda1, hda2]},
            "f2": {"batch": False, "values": [hda2, hda1]},
        }
    )
    execution.assert_has_job(0).with_output("out1").with_contents("1\t2\t3\n4\t5\t6\n")
    execution.assert_has_job(0).with_output("out2").with_contents("4\t5\t6\n1\t2\t3\n")

How to test the changes?

(Select all options that apply)

• This is a refactoring of components with existing test coverage.

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[jdavcs]

Out of curiosity, why do we need it to be a global?

[jmchilton]

Just recreating the semantics of the class version. More of a copy-paste thing then a choice I understand.

[jdavcs]

I suspect we may not need it. This celery_session_app is a celery fixture (celery app used for testing). But I'm not sure we use it. We are definitely not using _celery_app. And the celery_session_app fixture is being called regardless of that assignment statement. I know it's not part of this PR, but do you want to try and drop all that and leave just the yield statement in the try clause, and see what happens with the tests? I bet they'll pass.

[jmchilton]

Done - it all seems fine still.

[jdavcs]

This is very neat syntax; much better/concise/readable testing code! I'll be using it.