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

[DT-1072] Add Getz Lab data library #2739

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

fboulnois
Copy link
Contributor

Addresses

https://broadworkbench.atlassian.net/browse/DT-1072

Summary

Adds a Getz Lab data library that lives at /datalibrary/getzlab.


Have you read Terra's Contributing Guide lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@fboulnois fboulnois requested a review from a team as a code owner December 2, 2024 17:56
@fboulnois fboulnois requested review from pshapiro4broad and rjohanek and removed request for a team December 2, 2024 17:56
Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

👍🏽

Copy link
Contributor

@rjohanek rjohanek left a comment

Choose a reason for hiding this comment

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

looks good, is there any testing we want to do for this?

@fboulnois
Copy link
Contributor Author

looks good, is there any testing we want to do for this?

Yes, do you have any thoughts on how I might write a test like "if it's this particular Data Library, then expect to see this title"?

@rjohanek
Copy link
Contributor

rjohanek commented Dec 5, 2024

looks good, is there any testing we want to do for this?

Yes, do you have any thoughts on how I might write a test like "if it's this particular Data Library, then expect to see this title"?

Maybe you could use cypress.visit(url) to access the specific data library instead of using mountComponent like we usually do? If the specific library page info is stored via props to component, you could still use mountComponent, but I don't think it is right? so you might have to use visit to access the url directly. And then check for the title ? and maybe the filtered datasets too. I haven't done this before but I'd be happy to talk about it if you'd like! https://docs.cypress.io/api/commands/visit

@rushtong
Copy link
Contributor

rushtong commented Dec 6, 2024

looks good, is there any testing we want to do for this?

Yes, do you have any thoughts on how I might write a test like "if it's this particular Data Library, then expect to see this title"?

Maybe you could use cypress.visit(url) to access the specific data library instead of using mountComponent like we usually do? If the specific library page info is stored via props to component, you could still use mountComponent, but I don't think it is right? so you might have to use visit to access the url directly. And then check for the title ? and maybe the filtered datasets too. I haven't done this before but I'd be happy to talk about it if you'd like! https://docs.cypress.io/api/commands/visit

Using cy.visit would require a running instance (along with a backing consent, populated ES instance, etc) which makes that an integration test, not a unit test. We might be able to pass page parameters as arguments to the overall page, but I think that's a non-trivial effort and best handled as a distinct task in and of itself.

@rjohanek
Copy link
Contributor

rjohanek commented Dec 6, 2024

Using cy.visit would require a running instance (along with a backing consent, populated ES instance, etc) which makes that an integration test, not a unit test. We might be able to pass page parameters as arguments to the overall page, but I think that's a non-trivial effort and best handled as a distinct task in and of itself.

That makes sense, if we aren't looking to add integration tests then maybe you could mount the DatasetSearch component with the query (as a prop) that would be specified by this route and then check for the filtered datasets

@fboulnois
Copy link
Contributor Author

I'm going to create a separate ticket for unit tests of the data libraries to unblock this ticket.

@fboulnois fboulnois merged commit 42efee4 into develop Dec 9, 2024
9 checks passed
@fboulnois fboulnois deleted the fb-dt-1072-getz-lab-data-library branch December 9, 2024 15:12
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