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.

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

Jump to bottom

Add WDL unit tests to CI #5110

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

Add WDL unit tests to CI #5110

wants to merge 14 commits into from

Conversation

stxue1
Copy link
Contributor

@stxue1 stxue1 commented Oct 1, 2024 •
edited by adamnovak
Loading

Resolves #5109
Resolves #4941
Also makes the test_giraffe WDL test run with --logDebug so next time it fails similar to here it will give a more detailed answer. I think it is just a networking issues though, as mentioned here.

Also updates the commit to the branch here DataBiosphere/wdl-conformance-tests#46 (one of the arguments wasn't previously working, which that PR fixes.)

Changelog Entry

To be copied to the draft changelog by merger:

  • Added WDL unit tests to CI

Reviewer Checklist

  • Make sure it is coming from issues/XXXX-fix-the-thing in the Toil repo, or from an external repo.
    • If it is coming from an external repo, make sure to pull it in for CI with: 
      contrib/admin/test-pr otheruser theirbranchname issues/XXXX-fix-the-thing
    • If there is no associated issue, create one.
  • Read through the code changes. Make sure that it doesn't have:
    • Addition of trailing whitespace.
    • New variable or member names in camelCase that want to be in snake_case.
    • New functions without type hints.
    • New functions or classes without informative docstrings.
    • Changes to semantics not reflected in the relevant docstrings.
    • New or changed command line options for Toil workflows that are not reflected in docs/running/{cliOptions,cwl,wdl}.rst
    • New features without tests.
  • Comment on the lines of code where problems exist with a review comment. You can shift-click the line numbers in the diff to select multiple lines.
  • Finish the review with an overall description of your opinion.

Merger Checklist

  • Make sure the PR passes tests.
  • Make sure the PR has been reviewed since its last modification. If not, review it.
  • Merge with the Github "Squash and merge" feature.
    • If there are multiple authors' commits, add Co-authored-by to give credit to all contributing authors.
  • Copy its recommended changelog entry to the Draft Changelog.
  • Append the issue number in parentheses to the changelog entry.

DailyDreaming
DailyDreaming reviewed Oct 3, 2024
View reviewed changes
src/toil/test/wdl/wdltoil_test.py
@slow
def test_unit_tests_v11(self):
# There are still some bugs with the WDL spec, use a fixed version until
repo_url = "https://github.com/stxue1/wdl.git"
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... has this been made into a PR for openwdl?

@adamnovak adamnovak self-requested a review October 9, 2024 17:12
adamnovak
adamnovak requested changes Oct 11, 2024
View reviewed changes
Copy link
Member

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

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

There are some comments that I think need to be more specific, and it looks like we might not be properly handling failure from one of the test setup scripts.

src/toil/test/wdl/wdltoil_test.py
59, # will be fixed in #5001
66, # This needs way too many resources (and actually doesn't work?), see https://github.com/DataBiosphere/wdl-conformance-tests/blob/2d617b703a33791f75f30a9db43c3740a499cd89/README_UNIT.md?plain=1#L8
67, # same as above
68, # Bug
Copy link
Member

Choose a reason for hiding this comment

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

Which bug is this? How will we be able to tell when it is fixed?

src/toil/test/wdl/wdltoil_test.py
69, # Same as 68
87, # MiniWDL does not handle metacharacters properly when running regex, https://github.com/chanzuckerberg/miniwdl/issues/709
97, # miniwdl bug, see https://github.com/chanzuckerberg/miniwdl/issues/701
105, # miniwdl (and toil) bug, unserializable json is serialized
Copy link
Member

Choose a reason for hiding this comment

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

Is there a bug on MiniWDL to track this? If not we might want to open one.

src/toil/test/wdl/wdltoil_test.py
@@ -90,6 +114,17 @@ def check(self, p: subprocess.CompletedProcess) -> None:

p.check_returncode()

@slow
def test_unit_tests_v11(self):
# There are still some bugs with the WDL spec, use a fixed version until
Copy link
Member

Choose a reason for hiding this comment

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

This ought to indicate which issues need to be fixed before we can swap back to the official spec.

src/toil/test/wdl/wdltoil_test.py
Comment on lines +122 to +123
command = f"{exactPython} setup_unit_tests.py -v 1.1 --extra-patch-data unit_tests_patch_data.yaml --repo {repo_url} --branch {repo_branch} --force-pull"
p = subprocess.run(command.split(" "), capture_output=True)
Copy link
Member

Choose a reason for hiding this comment

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

check=True isn't set on this call, so it is allowed to fail. And capture_output=True is set but the output is never used, nor will it go to the same place as our output. So I think if this does fail it will fail silently.

This probably should be a check_call, or we should have a comment justifying why we want to completely ignore the failure of this command and any error messages it may produce.

Or maybe we're just missing another call to self.check(p)?

@adamnovak
Copy link
Member

This should fix #4941.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DailyDreaming DailyDreaming DailyDreaming left review comments

@adamnovak adamnovak adamnovak requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add WDL unit tests to CI Run wdl-tests, and add it to Toil CI
3 participants
@stxue1 @adamnovak @DailyDreaming