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

Add test for SchemaDefRequirement with a workflow and the types containing an $import statement #171

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

kinow
Copy link
Member

@kinow kinow commented May 13, 2022

For common-workflow-language/common-workflow-language#703

Used example written by @jeremiahsavage (thanks!). It doesn't output anything, but validates that it could import the types successfully.

Or if we prefer to test for some output, I can rewrite it to output something.

Cheers
Bruno

@mr-c
Copy link
Member

mr-c commented May 14, 2022

Or if we prefer to test for some output, I can rewrite it to output something.

It would be a better test if it did output something related to the custom types, yes.

@kinow
Copy link
Member Author

kinow commented May 14, 2022

Roger that, will keep that in mind for future PR's :-) thanks

@kinow kinow force-pushed the schemadef-types-with-import branch from 0a2c83f to 655f41a Compare May 16, 2022 05:46
- tool: tests/schemadef_types_with_import-wf.cwl
job: tests/schemadef_types_with_import-job.json
output: {
"out": "ordereddict([('CN', 'UNKNOWN'), ('DT', '1970-01-01T00:00:00Z'), ('ID', 'SRR622461'), ('LB', 'Illumina NA12878'), ('PI', '250'), ('PL', 'ILLUMINA'), ('SM', 'NA12878')])\n"
Copy link
Member Author

Choose a reason for hiding this comment

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

out here was interesting. In tests/schemadef_types_with_import-tool.cwl I used valueFrom: $(self.readgroup_meta_list) and it returned the repr of the ordered dict (I think).

If I used valueFrom: $(self.readgroup_meta_list)[] then the ordereddict prefix was gone 😕

What I wanted was to access DT or ID for the echo command. Any idea if that's doable?

Copy link
Member

Choose a reason for hiding this comment

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

That's a cwltool bug, the serialization should be a pure JSON object. Can you open an issue with a reproducer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Creating the smallest example to reproduce the issue, then will create the issue (and maybe a PR too).

Copy link
Member Author

Choose a reason for hiding this comment

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

- id: message
type: "schemadef_types_with_import_readgroup.yml#readgroups_bam_file"
inputBinding:
valueFrom: $(self.readgroup_meta_list)
Copy link
Member

Choose a reason for hiding this comment

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

If you want the value of DT, you could try this:

Suggested change
valueFrom: $(self.readgroup_meta_list)
valueFrom: $(self.readgroup_meta_list["DT"])

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 almost worked. But the error message displayed the read_group_meta_list that made me realize we had type: array in the input type. So $(self.readgroup_meta_list[0]['DT']) did the trick 👍 thanks @mr-c !

Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Either the cwltool serialization bug with ordereddict needs fixing, or the test needs to be change to not trigger that.

@kinow kinow force-pushed the schemadef-types-with-import branch from 655f41a to d37ea74 Compare June 23, 2022 03:56
…ining an $import statement. Used example written by @jeremiahsavage (thanks!)
@kinow
Copy link
Member Author

kinow commented Jun 23, 2022

Created cwltool issue, and the test output was updated. Ready for review again 👍

@kinow kinow requested a review from mr-c June 23, 2022 08:20
@mr-c mr-c merged commit f1f1002 into 1.2.1_proposed Jun 23, 2022
@mr-c mr-c deleted the schemadef-types-with-import branch June 23, 2022 08:21
@mr-c
Copy link
Member

mr-c commented Jun 23, 2022

Thanks!

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.

2 participants