-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Add testsuites hook #96494
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
base: master
Are you sure you want to change the base?
Add testsuites hook #96494
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #96494 +/- ##
==========================================
- Coverage 84.06% 77.41% -6.65%
==========================================
Files 10653 10651 -2
Lines 614683 614645 -38
Branches 24161 24159 -2
==========================================
- Hits 516731 475854 -40877
- Misses 97662 138501 +40839
Partials 290 290 |
PLACEHOLDER_TEST_SUITES.every(suite => value.includes(suite)); | ||
// Show 2 suites only if the combined string's length does not exceed 22. | ||
value.length === 0 || testSuites?.every(suite => value.includes(suite)); | ||
// Show 2 suites only if the combined string's length does not exceed MAX_SUITE_UI_LENGTH. |
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.
nit: we say doesn't exceed, but on line 103 we use < rather than <=
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.
Techincalllyyyy, you are right haha, changing it
@@ -50,9 +60,11 @@ export default function TestsPage() { | |||
<BranchSelector /> | |||
<DateSelector /> | |||
</PageFilterBar> | |||
<TestSuiteDropdown /> | |||
{(branch === undefined || branch === null || branch === defaultBranch) && ( |
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.
these first 2 checks are for the case where we don't have anything populated?
If so, can we get away with !branch || branch === defaultBranch?
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.
They're for the most part the same, although your approach will evaluate an empty string differently, but I'm comfortable w/ pushing your suggestion. I tend to go for readability over verbosity, but I think this is a case where it's okay
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.
yep yep thats right, as well as the "false" or number 0, but Idk if those are valid branch values here anyway
@@ -103,8 +118,9 @@ function Content() { | |||
|
|||
return ( | |||
<Fragment> | |||
{/* TODO: Conditionally show these if the branch we're in is the main branch */} | |||
<Summaries /> | |||
{(selectedBranch === undefined || |
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.
Similar story here with !selectedBranch || selectedBranch === defaultBranch
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.
lgtm!
This PR adds the test suite FE hook to fetch test suite related information.
Closes https://linear.app/getsentry/issue/CCMRG-1428/hideshow-summaries-based-on-branch-selection and https://linear.app/getsentry/issue/CCMRG-1426/ta-test-suite-dropdown
Notes
TestSuiteDropdown
componentuseTestSuites
hookuseGetTestResults
hook to further filter bytestSuites
if selecteduseInfinitedTestResults
hook to it's parent component to leverage defaultBranch field - hook is only enabled when all orgId, repo, branch and date are selected, so this avoids unnecessary, preemptive callstestSuites
when repo + org are changed