-
Notifications
You must be signed in to change notification settings - Fork 357
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
chore: tests for Searches component (ET-198) #9576
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9576 +/- ##
==========================================
- Coverage 51.36% 47.43% -3.93%
==========================================
Files 1252 929 -323
Lines 152174 123056 -29118
Branches 3024 3130 +106
==========================================
- Hits 78158 58369 -19789
+ Misses 73858 64534 -9324
+ Partials 158 153 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
* rerun bumpenvs after cherry-pick of #9576
Saw that some tests were failing in CI so I've reverted the dependency update, which still makes the I do think these warnings are not unique to this PR and are appearing throughout our test suite. It looks like this is something that's an issue in |
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.
do we also need to get 80% coverage for this kind of tests? right now, the cov is 47.43%
for the warnings, i think we can come back when upgrading the testing libraries
await waitFor(() => { | ||
expect(screen.getByText('2 searches')).toBeInTheDocument(); | ||
}); |
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.
await waitFor(() => { | |
expect(screen.getByText('2 searches')).toBeInTheDocument(); | |
}); | |
expect(await screen.findByText('2 searches')).toBeInTheDocument(); |
await waitFor(() => { | ||
expect(screen.getByText('0 searches')).toBeInTheDocument(); | ||
expect(screen.getByText('No Searches')).toBeInTheDocument(); | ||
}); |
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.
await waitFor(() => { | |
expect(screen.getByText('0 searches')).toBeInTheDocument(); | |
expect(screen.getByText('No Searches')).toBeInTheDocument(); | |
}); | |
expect(await screen.findByText('0 searches')).toBeInTheDocument(); | |
expect(await screen.findByText('No Searches')).toBeInTheDocument(); |
@@ -873,7 +875,16 @@ const Searches: React.FC<Props> = ({ project }) => { | |||
<div className={css.content} ref={contentRef}> | |||
{!isLoading && experiments.length === 0 ? ( | |||
numFilters === 0 ? ( | |||
<NoExperiments /> | |||
<Message |
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.
qq: what is the reason to replace <NoExperiments />
?
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.
Want it to say "Searches" instead of "Experiments"
I think code cov report is saying there's full coverage, the 47.43% is project-level coverage? Also I think a couple of the |
oh yes i always get confused
i tested them without |
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.
\o/
Ticket
ET-198
Description
Add tests to
Searches
componentTest Plan
No additional testing required, automated tests should pass.
Checklist
docs/release-notes/
See Release Note for details.