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

Testing improvements #44

Open
sckott opened this issue Jul 26, 2024 · 1 comment
Open

Testing improvements #44

sckott opened this issue Jul 26, 2024 · 1 comment
Labels
medium priority Important but not urgent, implement soon v2.0 Should be implemented in PROOF v2.0

Comments

@sckott
Copy link
Member

sckott commented Jul 26, 2024

It'd be nice to test with the latest few versions of Cromwell, e.g., https://github.com/broadinstitute/cromwell/releases/tag/87 came out in May, and it'd be nice to test this pkg with 86 and 87 at least.

For that to work easily, i'd like to make it so that tests could be run both:

  • with real http requests w/o manually doing so (right now it can take quite a while for a workflow to be available after submitting a job, so we have to sleep a while before running some other function to get some info about that job)
  • with vcr

Maybe it would work to somehow detect if we're inside a vcr::use_cassette block and vcr is turned off or on and behave accordingly, all in vcr::use_cassette block:

  • vcr is turned on & fixture IS NOT present: sleep/wait to make sure workflow/job info is available after cromwell_submit_batch calls
  • vcr is turned on & fixture IS present: no need to sleep/wait
  • vcr is turned off: sleep/wait after cromwell_submit_batch calls

The sleep/wait above could just be a Sys.sleep, or could be a retry situation with some backoff, or just same time gaps between each call until we get a 200 response

Once this is working, then idea is to have a matrix approach in the github actions yml file to test under different cromwell versions

@tefirman tefirman added medium priority Important but not urgent, implement soon v2.0 Should be implemented in PROOF v2.0 labels Jul 30, 2024
@tefirman
Copy link
Member

I feel very similarly to this issue as I do to #26 and #28, very important and could potentially be done now since rcromwell won't change much in v2.0 and the benefits would carry over, but it would require somewhat significant lift. Marking as v2.0 for now, but definitely could be convinced to put this in v1.X, especially if we can get all other high priority items done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium priority Important but not urgent, implement soon v2.0 Should be implemented in PROOF v2.0
Projects
None yet
Development

No branches or pull requests

2 participants