Missing outputs should be recorded as test errors #16094
Conversation
bernt-matthias
force-pushed
the
topic/assert-empty-output
branch
from
dc19da8 to
a0831b8
Compare
bernt-matthias
force-pushed
the
topic/assert-empty-output
branch
from
a0831b8 to
a9eeb09
Compare
bernt-matthias
force-pushed
the
topic/assert-empty-output
branch
from
a9eeb09 to
c414081
Compare
bernt-matthias
force-pushed
the
topic/assert-empty-output
branch
3 times, most recently
from
3a24616 to
288a6c9
Compare
|
A minor remark on the PR title: In general it is a good idea for the title to document what the PR does, instead of how you did something (which is pretty clear from the code anyway). So in this case I'd say something like "Make sure that missing outputs are recorded as test errors" helps a lot, both for the docs and the reviewer. |
|
@bernt-matthias I'm bumping this to 24.1. Feel free to re-milestone. |
- remove the output that is always present - add a test that has no outputs
from planemo the verify_tool function is called from [within an try-except block](https://github.com/galaxyproject/planemo/blob/1aa3eb05a97ad20c0be6f6560ab5cec090e76612/planemo/engine/galaxy.py#L109) which silently catches any exception. Thus any exception raised from within verify_tool will not be detected, i.e. the assertion needs to be moved into `_verify_outputs` (which also seems to make sense by name) in order to make verify_tool record the problem properly.
thereby we implicitly make the assumption that a test needs to produce an output also make message more specific
otherwise the tool test will just return a list out of bounds exception which is unclear to the user
bernt-matthias
force-pushed
the
topic/assert-empty-output
branch
from
e1c4cd6 to
012ab7d
Compare
| else: | ||
| output_data = data_list[len(data_list) - len(testdef.outputs) + output_index] | ||
| except IndexError: | ||
| pass |
There was a problem hiding this comment.
That seems weird, can you add a comment here ? I think that's also why the framework test failed.
There was a problem hiding this comment.
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.
then it might be clearer what's going on here
|
Should we target 24.0? |
jdavcs
changed the title
|
@bernt-matthias I'm merging this into dev - I don't think it's a bug (as per discussion above)? I've also edited the title - please feel free to change it if it's not a good summary of the PR. As per this comment, we'd have to manually edit this for the 24.1 release notes. |
|
It is definitely a bug, 24.0 is a good idea |
|
Not sure how to retarget without adding loads of commits? |
You should be able to edit the target: click the 'edit' button at the top of the page - it'll let you edit the title AND the base branch: you'll see a drop down list. Select '24.0 release' - it'll ask you to confirm and that's it. There may or may not be conflicts - is that what you were referring to? |
|
If I do this I get 215 commits. |
Oh no! Would you like to do #17873 instead of this (adding the extra commit to it)? i.e., merge it into 24.0 and then, if there are conflicts, manually forward-merge it into dev? |
|
All this is now in #17874 (had to create a new PR since I force pushed before reopening .. and apparently one cannot reopen in this case). |
from
planemotheverify_toolfunction is called from within an try-except block which silently catches any exception.Thus any exception raised from within verify_tool will not be detected, i.e. the assertion needs to be moved into
_verify_outputs(which also seems to make sense by name) in order to make verify_tool record the problem properly.This only affects tool tests. Guess we do not want to actually forbid tool runs with no outputs.. Even if I don't know what they are used for.
How to test the changes?
(Select all options that apply)
License