-
Notifications
You must be signed in to change notification settings - Fork 0
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
Adding cache test WDL and GitHub Action #66
base: main
Are you sure you want to change the base?
Conversation
Added @sitapriyamoorthi for review of the Cromwell/WDL aspects. If we feel like this shouldn't be its own GitHub Action, we can definitely explore other options, just let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tefirman sorry looking for some clarification here:
- Why do we need to modify the cromwell.conf? Can we not articulate this simple by having three WDLs with different options.json run in sequence with different caching configurations?
- Wouldnt modifying the cromwell.conf affect the other unit tests?
Sorry just trying to gain some clarity as to why we need to reconfigure cromwell.conf file?
@sitapriyamoorthi -- No worries, the Cromwell setup is super confusing honestly.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestions
- Write two separate unit tests:
- One for validating that outputs are properly written to the cache.
- Another for ensuring the workflow reads outputs from the cache when appropriate.
- Optionally, include a third combined test for end-to-end behavior to validate the full caching cycle.
Why Separate Tests?
-
Clarity: By isolating the read and write behaviors, you can clearly identify which part of the caching functionality is working or failing.
- A failure in a combined test could make it harder to pinpoint whether the issue lies in writing to the cache, reading from it, or both. Especially knowing that the cluster permission issues can also affect this.
-
Modular Testing: Unit tests should ideally focus on a single functionality or behavior.
- A separate test for writing ensures that the cache is being populated correctly.
- A separate test for reading ensures that previously cached results are being accessed as expected.
-
Debugging: If something goes wrong, separate tests make it easier to debug. For example:
- If the write-to-cache test fails, you know there's an issue with how the cache is being created or populated.
- If the read-from-cache test fails, it’s likely an issue with cache retrieval or matching logic.
-
Future Scalability: Separate tests make it easier to expand coverage for more complex caching scenarios in the future (e.g., testing caching with different runtime conditions or inputs).
I definitely agree with splitting these unit tests in concept. I will say that in practice, that becomes kinda difficult due to fresh environments for each GitHub Action, i.e. I can't read from cache without writing from the cache first. It's possible in theory though, let me give it a shot. I'm thinking the general approach will be:
Although, now I'm wondering if this would just be easier as one of the python-based "api-tests"... @sckott, penny-for-your-thoughts here? |
happy to think about it with you but don't totally understand - maybe we could chat about it on a call |
Description
Related Issues
Testing