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

Support importing WDL unit tests from spec #40

Merged
merged 38 commits into from
Sep 12, 2024
Merged

Conversation

stxue1
Copy link
Contributor

@stxue1 stxue1 commented Jul 3, 2024

The WDL spec unit tests should mostly be runnable (some may still have typos as I'm unable to fully check them due to miniwdl being partially broken).

The yaml representation for the original conformance tests have also changed a bit; failures are represented a little differently.

The dependency solution may be a bit iffy though, and one of the tests relies on having root access.

@stxue1 stxue1 marked this pull request as ready for review July 4, 2024 01:04
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.

This looks like it should mostly work, but there are some parts that seem easy to break and a lot of things I found confusing. If we're just going to sort of cheat on parts of the parsing we should note that with appropriate TODOs, or if we're doing something actually safe that looks like it won't generalize we should cite evidence somewhere that it won't need to.

lib.py Outdated Show resolved Hide resolved
lib.py Outdated Show resolved Hide resolved
lib.py Show resolved Hide resolved
lib.py Outdated Show resolved Hide resolved
lib.py Show resolved Hide resolved
setup_unit_tests.py Outdated Show resolved Hide resolved
setup_unit_tests.py Outdated Show resolved Hide resolved
setup_unit_tests.py Outdated Show resolved Hide resolved
unit_tests_metadata.yaml Outdated Show resolved Hide resolved
unit_tests_script.sh Outdated Show resolved Hide resolved
stxue1 and others added 8 commits August 19, 2024 21:10
Co-authored-by: Adam Novak <anovak@soe.ucsc.edu>
Co-authored-by: Adam Novak <anovak@soe.ucsc.edu>
Co-authored-by: Adam Novak <anovak@soe.ucsc.edu>
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.

It looks like some things have been fixed, and I understand some of what is going on better, but it looks like some things I flagged in the last review haven't been dealt with yet. Also I think I now understand why you're trying to use absolute paths under /mnt and I think you shouldn't be doing it.

README_UNIT.md Outdated Show resolved Hide resolved
lib.py Show resolved Hide resolved
lib.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
setup_unit_tests.py Outdated Show resolved Hide resolved
setup_unit_tests.py Outdated Show resolved Hide resolved
setup_unit_tests.py Outdated Show resolved Hide resolved
setup_unit_tests.py Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
stxue1 and others added 4 commits August 21, 2024 15:04
Co-authored-by: Adam Novak <anovak@soe.ucsc.edu>
Co-authored-by: Adam Novak <anovak@soe.ucsc.edu>
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.

I think this basically makes sense now.

setup_unit_tests.py Outdated Show resolved Hide resolved
if repeat is not None:
response["repeat"] = repeat
# Turn failing tests to warnings if they violate a test dependency
response.update(test_dependencies(dependencies=test.get("dependencies"), current_result=response))
Copy link
Member

Choose a reason for hiding this comment

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

If we're passing response in, and making this function return a modification, and then applying the modification, I think it gets kind of confusing.

We could make the function take the old response and return a new modified response copy. Or we could make it modify the response in place. Either of those would be simpler, and maybe easier to explain in the docstring.

run.py Outdated Show resolved Hide resolved
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