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

add csv data model to unit tests #1464

Merged
merged 4 commits into from
Aug 30, 2024
Merged

Conversation

lakikowolfe
Copy link
Contributor

@lakikowolfe lakikowolfe commented Aug 3, 2024

Include testing of csv data model in addition to jsonld

Related Jira ticket: https://sagebionetworks.jira.com/browse/FDS-1132

@lakikowolfe lakikowolfe requested a review from GiaJordan August 3, 2024 00:46
@lakikowolfe lakikowolfe requested a review from GiaJordan August 15, 2024 17:39
Copy link
Contributor

@GiaJordan GiaJordan left a comment

Choose a reason for hiding this comment

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

Looking through this again, it seems like the tests in test_manifest.py and test_validation.py are still only using the .jsonld could you update to test with the CSVs where the schemas module is concerned as well?

@linglp
Copy link
Contributor

linglp commented Aug 22, 2024

@lakikowolfe can ticket be linked to PR description? Also, when I queried the logs of manifest/generate, I didn't see any projects that are using .csv url when generating a manifest.. I am curious where the "ask" comes from...

@lakikowolfe
Copy link
Contributor Author

@linglp this is the ticket: https://sagebionetworks.jira.com/browse/FDS-1132

I believe the motivation behind this ticket is to update the tests to include csv because it is a supported file type and we'd want to know if for some reason csv input was not working.

@lakikowolfe
Copy link
Contributor Author

@GiaJordan My understanding is that this is low priority so I am focusing on the HTAN bug ticket that I have rn and will circle back to this when I can. Please let me know if I am misunderstanding the priority of this ticket and we can discuss.

@GiaJordan
Copy link
Contributor

@lakikowolfe yeah from my perspective your understanding is correct, but I'll loop in kevin so he can make the final determination

@GiaJordan
Copy link
Contributor

Looking through this again, it seems like the tests in test_manifest.py and test_validation.py are still only using the .jsonld could you update to test with the CSVs where the schemas module is concerned as well?
@lakikowolfe sorry about this earlier comment, looking at the current test_schemas.py I see it's already using both the csv and jsonld. you can disregard

@GiaJordan GiaJordan self-requested a review August 29, 2024 18:46
@GiaJordan
Copy link
Contributor

Approving pending resolution of merge conflicts

@thomasyu888 thomasyu888 self-requested a review August 29, 2024 18:56
Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

Great review @GiaJordan , and thanks for the work @lakikowolfe . Please answer some of my comments, but I'm going to go ahead and approve pending resolution of conflicts as Gianna stated

Copy link

@lakikowolfe lakikowolfe merged commit bb2606b into develop Aug 30, 2024
5 of 7 checks passed
@thomasyu888 thomasyu888 deleted the fds-1132-add-csv-to-unit-tests branch August 31, 2024 06:33
@GiaJordan GiaJordan restored the fds-1132-add-csv-to-unit-tests branch September 3, 2024 16:10
@GiaJordan GiaJordan deleted the fds-1132-add-csv-to-unit-tests branch September 3, 2024 16:10
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.

4 participants