Various extensions and improvements for asserts

[bernt-matthias]

Changes to text, size, tabular, archive, and xml assertions:

• Implement n, delta, min, max for all of them (previously only delta in has_size). I hope that we can save lots of test data files, since this can replace <output compare="sim_size">.
  • for xml only the new generalizing xml_element assertion and has_n_elements_with_path support n, delta, min, max
  • added new Bytes type to xsd to allow for bytes suffixes for all these nubers (might be useful also at other places)
• Implement negate attribute

Changes to has_n_columns

wanted to use a has_n_columns for this file https://github.com/galaxyproject/tools-iuc/blob/master/tools/stacks2/test-data/populations/populations.sumstats.tsv which is not useful due to the leading comment lines which have different numbers of columns. This PR adds a new attribute comment to the tag which allows to ignore comment lines for the assertion.

Furthermore:

• only non-empty lines are considered (this can be changed)

xml assertions

• add xml_element assertion which generalizes all other xml assertions
• all xml assertions are now implemented by a call to assert_xml_element

Tests and xsd changes

• adds unit tests for all assertions
• correctly represent assertions in xsd which allows for better linting
  • also show schema doc for assertions not as table but analogous to what is done for param (more readable), each with a table of allowed attributes
• annotate types (int and float) for assertion functions
• add linting of asserts

More

• fix a bug in element_text_is and attribute_is: they also allowed only a prefix matching

TODO (input needed):

• create a pr with the assert tests from dev to check if functionality has changed (only those that already worked, checking for len(x) == 1 to allow for reformulation)
• command, stdout and stderr assertions should maybe only allow text (possibly also tabular) assertions?
  • would break backward compatibility
• assertions with sub-assertions make no sense if negate=True
  • should be covered by linter
• would actually be cool to allow n, delta, min, max to allow for 500k, 50M, ...
• the introduced type annotations in the assertion functions are kind of odd, since the functions still get str (because the parser does not parse them to int,...),
  • but they are helpful for the linter which now uses the annotated types
  • which brings me to the question if the linter needs to check the types because they are now also annotated in xsd: -> xsd covers this .. is this enough?
• some of the text assertions check for None (i.e. this is currently not consistent) .. but I have no idea what it is good for

  galaxy/lib/galaxy/tool_util/verify/asserts/text.py

  Line 8 in 7335edd

  assert output is not None, "Checking has_text assertion on empty output (None)"

  .. we should use it in all or none of the assertions.
  • does it make sense to have this assert not affected by negate?
  • should be reflected in the xsd docs
  • This is most likely because the has_archive_member and element_text passed None to contained assertions. This has been changed .. so not necessary anymore
• add delta and min / max to all assertions having n/value
  • text, column, general
  • archive
  • xml
    • only for xml_element assertion and has_n_elements_with_path
• implement negate
  • text, column, general
  • archive
  • xml

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these contributions under Galaxy's current license.
• I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to galaxy-committers@lists.galaxyproject.org.

[jmchilton]

Amazing!

[simonbray]

I'm not sure I see the benefit of delta_frac for has_size and has_n_lines - I did consider implementing it at one point. Isn't it always possible to set delta to n/delta_frac, for whatever value of delta_frac you want? E.g. if n is 200 and you want delta_frac to be 10, you can set delta to 20.

For sim_size it is different because you do not necessarily know the value of n.

[bernt-matthias]

I'm not sure I see the benefit of delta_frac for has_size and has_n_lines - I did consider implementing it at one point. Isn't it always possible to set delta to n/delta_frac, for whatever value of delta_frac you want? E.g. if n is 200 and you want delta_frac to be 10, you can set delta to 20.

For sim_size it is different because you do not necessarily know the value of n.

You have a point here .. but for sim_size we also know the size of the file in test-data, or?

Maybe we can leave it because it makes it a bit easier for tool developers to switch from sim_size to assertions.

[bernt-matthias]

While we are at it we can also think about other useful extensions. Just thought that asserting a min/max number of lines/size/columns might be nice. But these could be separate new assertions as well.

[bernt-matthias]

Ping @jmchilton since you wrote this TODO

galaxy/lib/galaxy/tool_util/verify/asserts/__init__.py

Line 87 in 60cdf6b

# TODO: Verify all needed function arguments are specified.

I would like to ask if having a linter now for the asserts is sufficient, or if I should implement something along the lines of

galaxy/lib/galaxy/tool_util/linters/tests.py

Line 82 in 60cdf6b

assert_function_sig = signature(asserts.assertion_functions[assert_function_name])

[bernt-matthias]

@simonbray I removed delta_frac for now. But we it should be easy to readd.

For the sim_size test my rationale was: for test data of different sizes

• the lazy tool developer can copy paste assertions and use the same delta_frac unchanged
• even better it can be used in macros

.. delta would need to change with the size of the test data.

For the assertions the argumentation does not work that well. Because the expected size n/value needs to be set in the tool xml as well.

[simonbray]

I removed delta_frac for now. But we it should be easy to readd.

I guess it is no problem to add it, if it is more intuitive to use than the way I suggested.

[mvdbeek]

Do you think this is ready to merge @bernt-matthias ?

[bernt-matthias]

@mvdbeek .. nearly. I still need to execute my first TODO, i.e. check if I did not change functionality of the assertions: by creating a backport PR of the new linter tests (minus those that check new arguments)

Guess I can do this today and will ping you. Would also be handy for the other linter PR (guess this needs a bit of merging).

[jmchilton]

Amazing!

[bernt-matthias]

I opened a PR applying most of the new tests on the old assertion code #13241

There a few tests failing. In my opinion all of these cases are bugs of the old code .. but other people might think its change of functionality. So here are the cases and my explanation

element_text_is failure

The text of the ./elem/more tags is BAR and BAZ. So the
following should fail:

<assert_contents>
	<element_text_is path="./elem/more" text="BA"/>
</assert_contents>

It did not because

assert_element_text_is(output, path, text) just called
assert_element_text_matches(output, path, re.escape(text))
and the matching function allows for prefixes.

Now re.escape(text) + '$' is used which fixes the bug.

element_text failure

<element_text path="./absent"> was successful for the test XML
even though ./absent is not present in it.
This worked because the assertion function just applies the
contained assertions (here there is none) to the text.

I think in the old code this would also work if there
would be certain subassertions (exactly those that check
for None at first), i.e. this would be inconsistent.

We can easily change the new implementation to also pass
on non-existent paths (and not to check subassertions). But
I think its more convenient since otherwise the tool dev
needs to add an assertion making sure that path exists
to make this useful.

has_archive_member non-archive

Applying a has_archive_member assertion on a non-archive
file resulted in a UnboundLocalError, because the contents
variable did not exist if unzipping & untaring failed.

Now the assertion just fails.

has_archive_member zip absent member

Applying a has_archive_member using a path that is not in
the archive failed with an AttributeError because
the contents variable was None if unzipping and untarring
of the member failed (so contents.read() does not work).

has_archive_member tar absent member

Same as above

has_archive_member tar non-file member

Here the used path matches only for a directory in the zipfile.
The old code failed with the same error as the previous two cases,
i.e. AttributeError due to contents.read().

This is due to an inconsistency how zip and tar handle
directories. In zip they return the empty string,
in the new implementation of the assertion this is now
emulated for tar as well.

has_archive_member zip with content assertion

If there are multiple members matching the path (note that
the path is an regular expression) then the assertion
did check only the first returned.

Since the order of the matches was undefined this easily
fails, like here.

In the new implementation the matching members are sorted.
Also there is a new attribute all which allows to
check the subassertions on all matching members.

[mvdbeek]

Sooo, we merge ?

[bernt-matthias]

Fine with merging from my side.

[github-actions]

This PR was merged without a "kind/" label, please correct.