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

Tests For write_map and read_map #9

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

Conversation

power10dan
Copy link

Tests for write_map and read_map. pytest is having issues with md5sum, but the workflow output and expected output matches.

@@ -0,0 +1,9 @@
FROM ubuntu:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this dockerfile for?

Copy link
Author

Choose a reason for hiding this comment

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

This Dockerfile is for python image needed for running the task.

@@ -0,0 +1,25 @@
version 1.0
import "map_workflow.wdl" as map_wdl
Copy link
Contributor

Choose a reason for hiding this comment

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

Misleading name? The map_workflow.wdl doesn't seem to contain a workflow

Copy link
Author

Choose a reason for hiding this comment

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

Ooof, you are right. Let me get this fixed.

output {
File out_file_str = write_map.out1
File out_file_int = write_map.out2
File out_file_bool = write_map.out3
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you're trying to match the spacing, in which case you need one extra space on this line

Comment on lines 18 to 20
# Dan change: add serialization for
# type Map[File, Int] and Map[File, Bool]
# to make write_map more comprehensive
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this comment is still relevant?

@@ -0,0 +1,3 @@
/cromwell-executions/wf/c7e3bd7d-b00d-444e-8b3a-3fc0223d50d0/call-write_map/inputs/456912983/f1 alice
Copy link
Contributor

Choose a reason for hiding this comment

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

These files are not going to be on the same path in future runs. And in cloud runs these will be totally different.

If I were you I'd make these maps more deterministic by using something like Map[String, Int], rather than using the non-deterministic File

Copy link
Author

Choose a reason for hiding this comment

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

Got it. I shall change my test cases to something more deterministic.

@@ -0,0 +1,3 @@
{
"final_workflow_outputs_dir": "./tests/test_map_serialization/tests/data/workflow_output/"
Copy link
Contributor

Choose a reason for hiding this comment

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

workflow_options.json: is this (Cromwell) engine specific?

@@ -0,0 +1,3 @@
{
"final_workflow_outputs_dir": "./tests/test_map_serialization/tests/data/output_workflow"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file actually used?

@@ -0,0 +1,3 @@
f1 alice
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file ever used?

@@ -0,0 +1,3 @@
f1 true
Copy link
Contributor

Choose a reason for hiding this comment

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

This file looks like an output from a workflow run that can be deleted

@@ -0,0 +1,3 @@
f1 1
Copy link
Contributor

Choose a reason for hiding this comment

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

This file looks like an output from a workflow run that can be deleted

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