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

Missing outputs should be recorded as test errors #16094

Closed
wants to merge 12 commits into from
Closed

Missing outputs should be recorded as test errors #16094

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bda3ecf
adapt output filter test
bernt-matthias May 15, 2023
2e87de7
tool verification: move no output assertion
bernt-matthias May 15, 2023
a38e528
make test a bit more specific
bernt-matthias Jul 20, 2023
454d04a
register failure to produce output
bernt-matthias Jul 20, 2023
b1e69d9
register later
bernt-matthias Jul 20, 2023
012ab7d
move assertion instead of removing it
bernt-matthias Mar 25, 2024
bb7c082
Just use the suggestion
bernt-matthias Mar 25, 2024
ad249ef
check only presence of outputs mentioned in the tests
bernt-matthias Mar 25, 2024
11faeaa
Add back test output
bernt-matthias Mar 25, 2024
705ddca
Update test/functional/tools/output_filter.xml
bernt-matthias Mar 25, 2024
6ee5b09
move registering the exception
bernt-matthias Mar 26, 2024
3fe25d1
black
bernt-matthias Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 23 additions & 20 deletions lib/galaxy/tool_util/verify/interactor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1429,8 +1429,6 @@ def verify_tool(
raise e

if not expected_failure_occurred:
assert data_list or data_collection_list

try:
job_stdio = _verify_outputs(
testdef, test_history, jobs, data_list, data_collection_list, galaxy_interactor, quiet=quiet
Expand Down Expand Up @@ -1488,7 +1486,6 @@ def _handle_def_errors(testdef):
def _verify_outputs(testdef, history, jobs, data_list, data_collection_list, galaxy_interactor, quiet=False):
assert len(jobs) == 1, "Test framework logic error, somehow tool test resulted in more than one job."
job = jobs[0]

found_exceptions: List[Exception] = []

def register_exception(e: Exception):
Expand Down Expand Up @@ -1547,23 +1544,29 @@ def register_exception(e: Exception):
# Legacy - fall back on ordered data list access if data_list is
# just a list (case with twill variant or if output changes its
# name).
if hasattr(data_list, "values"):
output_data = list(data_list.values())[output_index]
else:
output_data = data_list[len(data_list) - len(testdef.outputs) + output_index]
assert output_data is not None
try:
galaxy_interactor.verify_output(
history,
jobs,
output_data,
output_testdef=output_testdef,
tool_id=job["tool_id"],
maxseconds=maxseconds,
tool_version=testdef.tool_version,
)
except Exception as e:
register_exception(e)
try:
if hasattr(data_list, "values"):
output_data = list(data_list.values())[output_index]
else:
output_data = data_list[len(data_list) - len(testdef.outputs) + output_index]
except IndexError:
pass
Copy link
Member

Choose a reason for hiding this comment

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

That seems weird, can you add a comment here ? I think that's also why the framework test failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. How about 6ee5b09? Hope this makes it clearer. The comment would have been more or less the content of the exception we are recording here.

if output_data:
try:
galaxy_interactor.verify_output(
history,
jobs,
output_data,
output_testdef=output_testdef,
tool_id=job["tool_id"],
maxseconds=maxseconds,
tool_version=testdef.tool_version,
)
except Exception as e:
register_exception(e)
else:
error = AssertionError(f"Tool did not produce an output with name '{name}' (or at index {output_index})")
register_exception(error)

other_checks = {
"command_line": "Command produced by the job",
Expand Down
88 changes: 33 additions & 55 deletions test/functional/tools/output_filter.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
<tool id="output_filter" name="output_filter" version="1.0.0">
<description>test for output filtering and expect_num_outputs</description>
<command><![CDATA[
echo 'test' > 1 &&
echo 'test' > 2 &&
echo 'test' > 3 &&
echo 'test' > 4 &&
echo 'test' > 5 &&
echo 'test' > p1.forward &&
echo 'test' > p1.reverse &&
echo 'test' > p2.forward &&
echo 'test' > p2.reverse
echo '1' > 1 &&
echo '2' > 2 &&
echo '3' > 3 &&
echo '4' > 4 &&
echo '5' > 5 &&
echo 'p1.forward' > p1.forward &&
bernt-matthias marked this conversation as resolved.
Show resolved Hide resolved
echo 'p1.reverse' > p1.reverse &&
echo 'p2.forward' > p2.forward &&
echo 'p2.reverse' > p2.reverse
]]></command>
<inputs>
<param name="produce_out_1" type="boolean" truevalue="true" falsevalue="false" checked="False" label="Do Filter 1" />
Expand All @@ -26,7 +26,6 @@ echo 'test' > p2.reverse
<!-- Must pass all filters... -->
<filter>filter_text_1 == "foo"</filter>
</data>
<data name="out_3" format="txt" from_work_dir="3"/>
<collection name="list_output" type="list" label="List">
<discover_datasets pattern="(?P&lt;identifier_0&gt;[45])" ext="txt" visible="true" />
<filter>produce_collection is True</filter>
Expand All @@ -37,134 +36,113 @@ echo 'test' > p2.reverse
</collection>
</outputs>
<tests>
<test expect_num_outputs="3">
<test expect_num_outputs="2">
<param name="produce_out_1" value="true" />
<param name="filter_text_1" value="foo" />
<output name="out_1">
<assert_contents>
<has_line line="test" />
<has_line line="1" />
</assert_contents>
</output>
<output name="out_2">
<assert_contents>
<has_line line="test" />
</assert_contents>
</output>
<output name="out_3">
<assert_contents>
<has_line line="test" />
<has_line line="2" />
</assert_contents>
</output>
</test>
<test expect_num_outputs="2">
<test expect_num_outputs="1">
<param name="produce_out_1" value="true" />
<param name="filter_text_1" value="bar" /> <!-- fails second filter in out2 -->
<output name="out_1">
<assert_contents>
<has_line line="test" />
</assert_contents>
</output>
<output name="out_3">
<assert_contents>
<has_line line="test" />
<has_line line="1" />
</assert_contents>
</output>
</test>
<test expect_num_outputs="1">
<!-- tool runs with no outputs should fail -->
bernt-matthias marked this conversation as resolved.
Show resolved Hide resolved
<test expect_num_outputs="0" expect_test_failure="true">
<param name="produce_out_1" value="false" />
<param name="filter_text_1" value="not_foo_or_bar" />
<output name="out_3">
<assert_contents>
<has_line line="test" />
</assert_contents>
</output>
<assert_stdout>
bernt-matthias marked this conversation as resolved.
Show resolved Hide resolved
<has_n_lines n="0"/>
</assert_stdout>
</test>
<test expect_num_outputs="4">
<test expect_num_outputs="3">
<param name="produce_out_1" value="true" />
<param name="filter_text_1" value="foo" />
<param name="produce_collection" value="true" />
<output name="out_1">
<assert_contents>
<has_line line="test" />
<has_line line="1" />
</assert_contents>
</output>
<output name="out_2">
<assert_contents>
<has_line line="test" />
</assert_contents>
</output>
<output name="out_3">
<assert_contents>
<has_line line="test" />
<has_line line="2" />
</assert_contents>
</output>
<output_collection name="list_output" type="list" count="2">
<element name="4">
<assert_contents>
<has_line line="test" />
<has_line line="4" />
</assert_contents>
</element>
<element name="5">
<assert_contents>
<has_line line="test" />
<has_line line="5" />
</assert_contents>
</element>
</output_collection>
</test>
<test expect_num_outputs="5">
<test expect_num_outputs="4">
<param name="produce_out_1" value="true" />
<param name="filter_text_1" value="foo" />
<param name="produce_collection" value="true" />
<param name="produce_paired_collection" value="true" />
<output name="out_1">
<assert_contents>
<has_line line="test" />
<has_line line="1" />
</assert_contents>
</output>
<output name="out_2">
<assert_contents>
<has_line line="test" />
</assert_contents>
</output>
<output name="out_3">
<assert_contents>
<has_line line="test" />
<has_line line="2" />
</assert_contents>
</output>
<output_collection name="list_output" type="list" count="2">
<element name="4">
<assert_contents>
<has_line line="test" />
<has_line line="4" />
</assert_contents>
</element>
<element name="5">
<assert_contents>
<has_line line="test" />
<has_line line="5" />
</assert_contents>
</element>
</output_collection>
<output_collection name="paired_list_output" type="list:paired" count="2">
<element name="p1">
<element name="forward">
<assert_contents>
<has_line line="test" />
<has_line line="p1.forward" />
</assert_contents>
</element>
<element name="reverse">
<assert_contents>
<has_line line="test" />
<has_line line="p1.reverse" />
</assert_contents>
</element>
</element>
<element name="p2">
<element name="forward">
<assert_contents>
<has_line line="test" />
<has_line line="p2.forward" />
</assert_contents>
</element>
<element name="reverse">
<assert_contents>
<has_line line="test" />
<has_line line="p2.reverse" />
</assert_contents>
</element>
</element>
Expand Down
Loading