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

Womtool and Cromwell test run pythonize #53

Merged
merged 10 commits into from
Jan 9, 2025
Merged

Womtool and Cromwell test run pythonize #53

merged 10 commits into from
Jan 9, 2025

Conversation

sckott
Copy link
Collaborator

@sckott sckott commented Jan 3, 2025

Attempt to move some logic from gh action for womtool validate to python pytest. Motivating reason was that we expect some wdl's to fail and it's easier to do that in python

Key points

  • action file retains the matrix approach where each wdl folder/file is a separate run
  • we pass in the file for each matrix run to the test script
  • test-validate.py:
    • line 3 has the list of wdl folders/files that we expect to fail so that we don't fail the run when womtool fails
    • very simple test expectations so far. can add more, any suggestions?
    • Since this PR focuses on the womtool testing, should we ignore the failing "Cromwell Test Run" situation? Or address that here? It's a result of including an on purporse bad file
  • Do you like this approach or keep the logic in the action yml file?

@sckott sckott requested review from seankross and tefirman January 6, 2025 14:30
@seankross
Copy link
Collaborator

line 3 has the list of wdl folders/files that we expect to fail so that we don't fail the run when womtool fails

I wonder if we should have a naming convention for test wdls, such that if the wdl name starts with bad- or fail- then it's expected to fail, instead of keeping track of each wdl that is supposed to fail.

very simple test expectations so far. can add more, any suggestions?

@tefirman world know better than me in terms of what to test in the womtools case.

Since this PR focuses on the womtool testing, should we ignore the failing "Cromwell Test Run" situation? Or address that here? It's a result of including an on purporse bad file

I've thought about this, that we're repeated ourselves in having two separate gha files for basic cromwell-on-gha and womtools, I am in favor of consolidating them unless @tefirman knows a good reason we should keep them separate.

Do you like this approach or keep the logic in the action yml file?

I like this approach! It seems we have much more control without adding too much more complexity.

@sckott
Copy link
Collaborator Author

sckott commented Jan 8, 2025

@seankross

wonder if we should have a naming convention for test wdls

Yeah that's the other way to go I think. Happy to do that instead.

@tefirman
Copy link
Collaborator

tefirman commented Jan 8, 2025

very simple test expectations so far. can add more, any suggestions?

This baseline is perfect for now. Eventually, I'd like to check the outputs of the test run to ensure the results are what we expect for the successful runs and the error messages are what we expect for the bad runs, but let's add that in a subsequent PR.

I've thought about this, that we're repeated ourselves in having two separate gha files for basic cromwell-on-gha and womtools, I am in favor of consolidating them unless @tefirman knows a good reason we should keep them separate.

I think it makes sense to consolidate as long as it's just the test run, i.e. no womtools, because whatever gets caught by WOMtools will get caught by the test run, but not necessarily the other way around.

I wonder if we should have a naming convention for test wdls, such that if the wdl name starts with bad-

I'm very pro-naming-convention, feels more visible and obvious compared to a list within the yml file.

Do you like this approach or keep the logic in the action yml file?

REALLY like this approach as well! Much more flexible than the action yml alone!

@sckott
Copy link
Collaborator Author

sckott commented Jan 8, 2025

This baseline is perfect for now

Okay, sounds good. Issue opened 👉🏽 #54

I think it makes sense to consolidate ...

@tefirman @seankross do you mean have one yml gh action file combining the actions named "Cromwell Test Run" and "WOMtool Validation of WDL Script"?

naming convention ...

Okay, i'll go with bad unless someone speaks up otherwise

@tefirman
Copy link
Collaborator

tefirman commented Jan 8, 2025

Okay, sounds good. Issue opened 👉🏽 #54

Perfect, thanks!

do you mean have one yml gh action file combining the actions named "Cromwell Test Run" and "WOMtool Validation of WDL Script"?

I think I'm suggesting skipping WOMtools altogether in this context. It's useful for more complex workflow so you don't have to wait forever for the workflow to run all the way through, but these are so simple that it's unnecessary. @seankross , do you agree?

Okay, i'll go with bad unless someone speaks up otherwise

Sounds ideal to me!

@seankross
Copy link
Collaborator

I think I'm suggesting skipping WOMtools altogether in this context. It's useful for more complex workflow so you don't have to wait forever for the workflow to run all the way through, but these are so simple that it's unnecessary. @seankross , do you agree?

Oh I disagree, I think we should have the womtools check for all wdls just to eliminate something womtools would catch as a source of error. Also, more complex workflows are going to make their way into this testing regime soon.

@tefirman @seankross do you mean have one yml gh action file combining the actions named "Cromwell Test Run" and "WOMtool Validation of WDL Script"?

On second thought let's keep them separate.

Okay, i'll go with bad unless someone speaks up otherwise

bad sounds good 😝

I'm very pro-naming-convention, feels more visible and obvious compared to a list within the yml file.

yeah let's do this

@sckott
Copy link
Collaborator Author

sckott commented Jan 8, 2025

Thanks friends. SO

  • using a list of bad files is out and naming convention of prefix bad is in
  • keep current womtool and cromwell test run gh action yml's separate
  • not discussed per se, but I'll pythonize the cromwell test run stuff too so that this PR is passing checks and is a more cohesive piece of work

@sckott sckott changed the title Womtool pythonize Womtool and Cromwell test run pythonize Jan 8, 2025
@seankross
Copy link
Collaborator

Love it, thank you!

@tefirman
Copy link
Collaborator

tefirman commented Jan 8, 2025

Perfect, thanks @sckott !! 🙏

Copy link
Collaborator

@seankross seankross left a comment

Choose a reason for hiding this comment

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

Looks great, thank you.

@sckott sckott merged commit aa79b43 into main Jan 9, 2025
11 checks passed
@sckott sckott deleted the womtool-pythonize branch January 9, 2025 23:13
@tefirman
Copy link
Collaborator

tefirman commented Jan 9, 2025

This is awesome, @sckott , thanks for adding this!

My only comment is that eventually we might want to split the "bad" verbiage between WOMtools and TestRun somehow. For instance, we might have a unit test that passes WOMtools validation, but fails TestRun. Not crucial right now, just wanted to write the thought down for posterity's sake.

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.

3 participants