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

Adding array operations unit test WDL #25

Merged
merged 10 commits into from
Jan 14, 2025
Merged

Adding array operations unit test WDL #25

merged 10 commits into from
Jan 14, 2025

Conversation

tefirman
Copy link
Collaborator

@tefirman tefirman commented Dec 17, 2024

Description

  • Adding an "Array Operations" workflow that tests array operations and scatter-gather functionality by converting an array of strings to uppercase.

Related Issues

Testing

  • Ran locally using miniwdl, succeeds.

@tefirman tefirman marked this pull request as ready for review December 17, 2024 00:55
@tefirman tefirman requested a review from seankross December 17, 2024 00:55
@sitapriyamoorthi
Copy link
Collaborator

@tefirman this looks great! Wondering if we should add tests that specifically evaluate:

  1. Empty arrays as inputs
  2. A way to ensure elements of an array are correctly indexed
  3. Verifying that the length, flatten and sort functions applied to arrays works
  4. Maybe test array concatenation (overkill?)
  5. Testing array of arrays works

Just some thoughts... feel free to comment on if all or some of these will be useful!

@seankross seankross added the unit test Adding or modifying a unit test label Dec 18, 2024
@tefirman
Copy link
Collaborator Author

tefirman commented Dec 19, 2024

@sitapriyamoorthi -- Great call. I added a few more tasks to implement your suggestions:

  • ValidateIndex
  • ArrayFunctions
  • ArrayConcat
  • Array of arrays as an input

Not super worried about empty arrays, but I did run it with an empty array and it seems to work fine. Mind taking another look?

Copy link
Collaborator

@sitapriyamoorthi sitapriyamoorthi left a comment

Choose a reason for hiding this comment

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

Hi @tefirman thanks for including those changes. Just a couple of things that would complete this unit test :

  1. Could you add to test an array of integers (just so we complete the circle). And test that the integers are properly parsed (my assumption is something simple like addition of integers would test that)
  2. Could you also add something that tests the intermediate declaration of an array of integers similar to what you have done for strings.
  3. Could you also add a small part to test that an array of files are properly localized and can be correctly indexed.

Thanks!

@tefirman
Copy link
Collaborator Author

tefirman commented Jan 8, 2025

@sitapriyamoorthi -- added a few updates to address your suggestions:

  1. See IntegerArrayOps task.
  2. See more_numbers intermediate variable.
  3. See FileArrayOps task.

Mind taking another look?

@sitapriyamoorthi
Copy link
Collaborator

Been reviewing it... will add my comments shortly..

sitapriyamoorthi and others added 4 commits January 8, 2025 14:47
Updating the Readme with small changes to make things a bit more clear
Adding comments for clarity.
@tefirman
Copy link
Collaborator Author

tefirman commented Jan 9, 2025

@sitapriyamoorthi -- Merged in your PR if you wanted to take another look.

@sitapriyamoorthi
Copy link
Collaborator

Looks good! @tefirman

@sitapriyamoorthi
Copy link
Collaborator

@tefirman I am holding back from squashing and merging just yet since technically I have not successfully got this to run on PROOF. Starting a thread on the proof channel to discuss this. My thoughts are that since it runs locally on miniwdl. The WDL itself works on a WDL runner but may or may not run on PROOF. Lets chat more there before merging.

@tefirman
Copy link
Collaborator Author

@sitapriyamoorthi -- Just confirmed that this WDL runs successfully in the proof-api-dev branch, so I'm going to merge this into main.

@tefirman tefirman merged commit 5de2154 into main Jan 14, 2025
13 checks passed
@tefirman tefirman deleted the array-unit-test branch January 14, 2025 19:52
@sitapriyamoorthi sitapriyamoorthi linked an issue Jan 15, 2025 that may be closed by this pull request
@sitapriyamoorthi sitapriyamoorthi linked an issue Jan 15, 2025 that may be closed by this pull request
@sitapriyamoorthi sitapriyamoorthi linked an issue Jan 15, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unit test Adding or modifying a unit test
Projects
None yet
3 participants