Skip to content

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

Merged
merged 4 commits into from
May 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 92 additions & 10 deletions static/app/views/codecov/tests/onboarding.spec.tsx
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: ''},
},
Copy link
Contributor

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

},
});

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');
});
});
66 changes: 64 additions & 2 deletions static/app/views/codecov/tests/onboarding.tsx
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();
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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>
);
}
Expand All @@ -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)};
`;
Loading