-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(codecov): Add onboarding radio selector for GHA or CLI #90980
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
Changes from all commits
733aa36
163a5f4
afe4b07
e8695d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,100 @@ | ||
import {OrganizationFixture} from 'sentry-fixture/organization'; | ||
import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary'; | ||
|
||
import {render, screen} from 'sentry-test/reactTestingLibrary'; | ||
import TestsOnboardingPage from 'sentry/views/codecov/tests/onboarding'; | ||
|
||
import TestAnalyticsOnboardingPage from 'sentry/views/codecov/tests/onboarding'; | ||
describe('TestsOnboardingPage', () => { | ||
it('renders with GitHub Actions selected by default if no query param is provided', () => { | ||
render(<TestsOnboardingPage />, { | ||
initialRouterConfig: { | ||
location: { | ||
pathname: '/codecov/tests/new', | ||
query: {}, | ||
}, | ||
}, | ||
}); | ||
|
||
const githubRadio = screen.getByLabelText('Use GitHub Actions to run my CI'); | ||
expect(githubRadio).toBeChecked(); | ||
|
||
const COVERAGE_FEATURE = 'codecov-ui'; | ||
const cliRadio = screen.getByLabelText( | ||
"Use Sentry Prevent's CLI to upload testing reports" | ||
); | ||
expect(cliRadio).not.toBeChecked(); | ||
}); | ||
|
||
describe('TestAnalyticsOnboardingPage', () => { | ||
it('renders the placeholder content', () => { | ||
render(<TestAnalyticsOnboardingPage />, { | ||
organization: OrganizationFixture({features: [COVERAGE_FEATURE]}), | ||
it('renders with GitHub Actions selected by default if empty opt query param is provided', () => { | ||
render(<TestsOnboardingPage />, { | ||
initialRouterConfig: { | ||
location: { | ||
pathname: '/codecov/tests/new', | ||
query: {opt: ''}, | ||
}, | ||
}, | ||
}); | ||
|
||
const testContent = screen.getByText('Test Analytics Onboarding'); | ||
expect(testContent).toBeInTheDocument(); | ||
const githubRadio = screen.getByLabelText('Use GitHub Actions to run my CI'); | ||
expect(githubRadio).toBeChecked(); | ||
|
||
const cliRadio = screen.getByLabelText( | ||
"Use Sentry Prevent's CLI to upload testing reports" | ||
); | ||
expect(cliRadio).not.toBeChecked(); | ||
}); | ||
|
||
it('renders with CLI selected when opt=cli in URL', () => { | ||
render(<TestsOnboardingPage />, { | ||
initialRouterConfig: { | ||
location: { | ||
pathname: '/codecov/tests/new', | ||
query: {opt: 'cli'}, | ||
}, | ||
}, | ||
}); | ||
|
||
const cliRadio = screen.getByLabelText( | ||
"Use Sentry Prevent's CLI to upload testing reports" | ||
); | ||
expect(cliRadio).toBeChecked(); | ||
|
||
const githubRadio = screen.getByLabelText('Use GitHub Actions to run my CI'); | ||
expect(githubRadio).not.toBeChecked(); | ||
}); | ||
|
||
it('updates URL when GitHub Actions option is selected', async () => { | ||
const {router} = render(<TestsOnboardingPage />, { | ||
initialRouterConfig: { | ||
location: { | ||
pathname: '/codecov/tests/new', | ||
query: {opt: 'cli'}, | ||
}, | ||
}, | ||
}); | ||
|
||
const githubRadio = screen.getByLabelText('Use GitHub Actions to run my CI'); | ||
expect(githubRadio).not.toBeChecked(); | ||
|
||
await userEvent.click(githubRadio); | ||
|
||
expect(router.location.search).toBe('?opt=githubAction'); | ||
}); | ||
|
||
it('updates URL when CLI option is selected', async () => { | ||
const {router} = render(<TestsOnboardingPage />, { | ||
initialRouterConfig: { | ||
location: { | ||
pathname: '/codecov/tests/new', | ||
query: {opt: ''}, | ||
}, | ||
}, | ||
}); | ||
|
||
const cliRadio = screen.getByLabelText( | ||
"Use Sentry Prevent's CLI to upload testing reports" | ||
); | ||
expect(cliRadio).not.toBeChecked(); | ||
|
||
await userEvent.click(cliRadio); | ||
|
||
expect(router.location.search).toBe('?opt=cli'); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,47 @@ | ||
import {useCallback} from 'react'; | ||
import {useSearchParams} from 'react-router-dom'; | ||
import styled from '@emotion/styled'; | ||
|
||
import RadioGroup from 'sentry/components/forms/controls/radioGroup'; | ||
import {t} from 'sentry/locale'; | ||
import {space} from 'sentry/styles/space'; | ||
import TestPreOnboardingPage from 'sentry/views/codecov/tests/preOnboarding'; | ||
|
||
type SetupOption = 'githubAction' | 'cli'; | ||
|
||
export default function TestsOnboardingPage() { | ||
const [searchParams, setSearchParams] = useSearchParams(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious as to why we're using the url to handle state vs a local useState. Are we expecting to reach the /new route with a url param that could be any of these two? And what is the value of exposing the opt to the customer via the URL vs having it in an internal state? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm...I swear I read somewhere that a requirement was to have the url change so that linking to it would preset the option. Now I'm having a hard time finding where I saw that though... Gonna have to do a little bit of digging There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, it was on this ticket https://linear.app/getsentry/issue/CCMRG-150/test-analytics-onboarding-create-radio-selection. Seems like we may have two similar/duplicate tickets for the radio selector that were created at different times. This is the other one I have included in the PR description https://linear.app/getsentry/issue/CCMRG-426/add-radio-selector-for-choosing-between-gh-actions-and-cli There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh gotcha gotcha, ok sounds good! Yeap if this is expected to be part of the URL then this approach is good. Should the url be populated with the default value if one isn't provided? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you see any benefits we would get with the default value being added? I was thinking of setting the searchParams on mount to add the query param by default but that forces a remount which I think may not be worth it if it already works when no default is specified. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly it being in sync with the app's state. When you click on another entry it syncs, which is nice, but I suppose if you default to an option it's okay as long as there's a default for blank. |
||
const opt = searchParams.get('opt'); | ||
|
||
const handleRadioChange = useCallback( | ||
(newOption: SetupOption) => { | ||
setSearchParams({opt: newOption}); | ||
}, | ||
[setSearchParams] | ||
); | ||
|
||
return ( | ||
<LayoutGap> | ||
<p>Test Analytics Onboarding</p> | ||
<TestPreOnboardingPage /> | ||
<OnboardingContainer> | ||
<IntroContainer> | ||
<GetStartedHeader>{t('Get Started with Test Analytics')}</GetStartedHeader> | ||
<TAValueText> | ||
{t( | ||
'Test Analytics offers data on test run times, failure rates, and identifies flaky tests to help decrease the risk of deployment failures and make it easier to ship new features quickly.' | ||
)} | ||
</TAValueText> | ||
</IntroContainer> | ||
<SelectOptionHeader>{t('Select a setup option')}</SelectOptionHeader> | ||
<RadioGroup | ||
label="Select a setup option" | ||
value={opt === 'cli' ? 'cli' : 'githubAction'} | ||
onChange={handleRadioChange} | ||
choices={[ | ||
['githubAction', t('Use GitHub Actions to run my CI')], | ||
['cli', t("Use Sentry Prevent's CLI to upload testing reports")], | ||
]} | ||
/> | ||
</OnboardingContainer> | ||
</LayoutGap> | ||
); | ||
} | ||
|
@@ -16,3 +50,31 @@ const LayoutGap = styled('div')` | |
display: grid; | ||
gap: ${space(2)}; | ||
`; | ||
|
||
const OnboardingContainer = styled('div')` | ||
padding: ${space(1.5)} ${space(4)}; | ||
border: 1px solid ${p => p.theme.border}; | ||
border-radius: ${p => p.theme.borderRadius}; | ||
`; | ||
|
||
const IntroContainer = styled('div')` | ||
border-bottom: 1px solid ${p => p.theme.border}; | ||
padding-bottom: ${space(3)}; | ||
`; | ||
|
||
const GetStartedHeader = styled('h2')` | ||
font-size: 1.625rem; | ||
color: ${p => p.theme.tokens.content.primary}; | ||
line-height: 40px; | ||
`; | ||
|
||
const TAValueText = styled('p')` | ||
font-size: ${p => p.theme.fontSizeLarge}; | ||
color: ${p => p.theme.tokens.content.primary}; | ||
`; | ||
|
||
const SelectOptionHeader = styled('h5')` | ||
font-size: ${p => p.theme.fontSizeExtraLarge}; | ||
color: ${p => p.theme.tokens.content.primary}; | ||
margin-top: ${space(3)}; | ||
`; |
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.
I think it'd be worth adding a test with query being empty/null; subtle but different is that the radio button will have the default value (gha) but the opt will be '' (or undefined if it's just
/new
), leaving the url and button out of sync. Sort of tied to my other comment of having the url be the state vs a useState hook