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.

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

Validate with jsonschema #1417

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Dec 8, 2023

also

  • fixes the wrong and extremely verbose input connection check (we used to print every step dict!)
  • fixes gxformat2 linting if gxformat2 steps are lists and not dicts
  • make unknown job inputs an error (cause you may think you're testing something that you're not)
  • allow elements in top level collection tests in addition to element_tests (i've marked element_tests as deprecated in the schema)

The schema disallows the - that: has_text style assertions. They continue to work in tests but results in linting errrors, I hope that's ok. I've included the schema for now in planemo, but eventually we should either include it in galaxy-tool-util (as soon as pydantic 2 can be supported) or just publish the schema to https://www.schemastore.org/json/. The process seems easy enough, but we may need to come up with a more unique test file name (*.gxwf-test.yml) ?

@mvdbeek mvdbeek requested a review from jmchilton December 8, 2023 17:47
@jmchilton
Copy link
Member

Can you document some where how planemo/schema/test_file_schema.json is generated? Is that present and I missed it?

@@ -21,10 +21,10 @@
text: Word
second:
asserts:
- that: has_text
Copy link
Member

@jmchilton jmchilton Dec 8, 2023

Choose a reason for hiding this comment

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

Does the version of this where these isn't a list - but it is just a dictionary keyed on the assertion types validate? That seems to be what we have documented in: https://planemo.readthedocs.io/en/latest/test_format.html#test-format. I assume yes but I wanted to make sure.

I definitely prefer the a list of flat dictionaries with the fixed that keyword to a list of nested dictionaries each with a single outer key. The structure of this feels very... awkward or forced. Is this really the syntax you prefer or is JSON schema coercing us in this direction?

"additionalProperties": false,
"description": "Asserts the output has a specific size (in bytes) of ``value`` plus minus\n``delta``, e.g. ``<has_size value=\"10000\" delta=\"100\" />``.\n\nAlternatively the range of the expected size can be specified by ``min`` and/or\n``max``.\n$attribute_list::5",
"properties": {
"value": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add size ass synonym for value (as in galaxyproject/galaxy#17490)

Comment on lines +218 to +224
"has_size": {
"items": {
"$ref": "#/$defs/AssertHasSize"
},
"title": "Has Size",
"type": "array"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having all these assertions as properties we should have "children", which contain a list of assertions, or.

At least that's what the parser does : https://github.com/galaxyproject/galaxy/blob/9ddb6ad44afc59fadec39f5bdbf8e6b106774734/lib/galaxy/tool_util/parser/yaml.py#L291 .. or lets say is supposed to do (since the recursion is missing at the moment .. I guess this is easy to fix and I will try).

In hindsight it might have been better to just use "asserts" instead of "children" since its also the name of the top level list of asserts (or is this only in workflow tests).

Copy link
Member Author

Choose a reason for hiding this comment

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

Just need to regenerate the schema, it's all derived from the xsd

Copy link
Member Author

Choose a reason for hiding this comment

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

That was a comment on #1417 (comment) ...

I am not sure I understand the proposition here, this doesn't seem necessary as you can simply use a list of assertions ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants