Skip to content

[23.2] Yaml nested assertions: fix parsing #17641

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

Merged
mvdbeek merged 5 commits into from
Mar 11, 2024
Merged

[23.2] Yaml nested assertions: fix parsing #17641

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2a7cce0
yaml parsing: fix nested assertions
bernt-matthias Mar 8, 2024
df7c2af
typing fix
bernt-matthias Mar 8, 2024
e76f5ff
adad test to allow for Optional list
bernt-matthias Mar 8, 2024
9a17a04
fix: nested assertions must be list
bernt-matthias Mar 8, 2024
5a48f66
use asserts also in test
bernt-matthias Mar 9, 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
2 changes: 1 addition & 1 deletion lib/galaxy/tool_util/parser/interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
class AssertionDict(TypedDict):
tag: str
attributes: Dict[str, Any]
children: Optional[List[Dict[str, Any]]]
children: "AssertionList"


AssertionList = Optional[List[AssertionDict]]
Expand Down
5 changes: 1 addition & 4 deletions lib/galaxy/tool_util/parser/yaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,7 @@ def expand_dict_form(item):
new_assertion["that"] = assertion_key
new_assertion.update(assertion_value)
assertion = new_assertion
children = []
if "children" in assertion:
children = assertion["children"]
del assertion["children"]
children = to_test_assert_list(assertion.pop("children", []))
assert_dict: AssertionDict = dict(
tag=assertion["that"],
attributes=assertion,
Expand Down
23 changes: 23 additions & 0 deletions test/unit/tool_util/test_test_parsing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,26 @@ def test_simple_assert_to_test_assert_list():

def test_assert_legacy_same_as_new_list_style():
assert to_test_assert_list(ASSERT_THAT_LIST) == to_test_assert_list(ASSERT_THAT_LIST)


NESTED_ASSERT_LIST = yaml.safe_load(
"""
- has_archive_member:
path: ".*"
children:
Copy link
Copy Markdown
Member

@mvdbeek mvdbeek Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess that's what your comment about asserts was about ? It does feel more consistent for that to be

asserts:
  - has_text:
    text: thing

?

Copy link
Copy Markdown
Contributor Author

@bernt-matthias bernt-matthias Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I used the syntax from the other examples in the file

I'm still struggling with yaml, but it seems that

yaml.load("""
asserts:
  - has_text:
    text: thing
"""
)

gives `{'asserts': [{'has_text': None, 'text': 'thing'}]}`

while

```python
yaml.load("""
asserts:
  - has_text:
      text: thing
"""
)

gives {'asserts': [{'has_text': {'text': 'thing'}}]}. Not sure if both versions are parsed at the moment.

Not sure if the former even works.

Copy link
Copy Markdown
Member

@mvdbeek mvdbeek Mar 8, 2024

Choose a reason for hiding this comment

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

Sorry, typing without checking, it should be

asserts:
  - has_text:
      text: thing

Copy link
Copy Markdown
Contributor Author

@bernt-matthias bernt-matthias Mar 8, 2024

Choose a reason for hiding this comment

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

Yes this is what I meant with my comment at the planemo issue

Not sure, but easy to implement I guess. But I can clearly live with the current state. The only question would be if it would simplify anything if we use asserts?

Copy link
Copy Markdown
Member

@mvdbeek mvdbeek Mar 8, 2024

Choose a reason for hiding this comment

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

Yes, then we can re-use models and there's no question what you have to use IMO - it's always asserts.

Copy link
Copy Markdown
Contributor Author

@bernt-matthias bernt-matthias Mar 8, 2024

Choose a reason for hiding this comment

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

Then remove children completely, or leave it as falll back? I guess since it seems that this was never used, we may just change it?

Copy link
Copy Markdown
Member

@mvdbeek mvdbeek Mar 8, 2024

Choose a reason for hiding this comment

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

Leave it as a fallback but don't document it would be my suggestion ?

- has_text:
text: "a text"
- has_text:
text: "another text"
"""
)


def test_nested_asserts():
asserts = to_test_assert_list(NESTED_ASSERT_LIST)
assert asserts and len(asserts) == 1
assert asserts and asserts[0]["children"] and len(asserts[0]["children"]) == 2
assert asserts and asserts[0]["children"] and asserts[0]["children"][0]["tag"] == "has_text"
assert asserts and asserts[0]["children"] and asserts[0]["children"][0]["attributes"]["text"] == "a text"
assert asserts and asserts[0]["children"] and asserts[0]["children"][1]["tag"] == "has_text"
assert asserts and asserts[0]["children"] and asserts[0]["children"][1]["attributes"]["text"] == "another text"