Add a placeholder Component Tab #2731
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #2731 +/- ##
==========================================
- Coverage 98.42% 98.37% -0.06%
==========================================
Files 857 872 +15
Lines 12261 12462 +201
Branches 3240 3242 +2
==========================================
+ Hits 12068 12259 +191
- Misses 188 194 +6
- Partials 5 9 +4
... and 1 file with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #2731 +/- ##
==========================================
- Coverage 98.42% 98.37% -0.06%
==========================================
Files 857 872 +15
Lines 12261 12462 +201
Branches 3221 3305 +84
==========================================
+ Hits 12068 12259 +191
- Misses 188 194 +6
- Partials 5 9 +4
... and 1 file with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found Additional details and impacted files@@ Coverage Diff @@
## main #2731 +/- ##
=======================================
- Coverage 98.43 98.37 -0.06
=======================================
Files 857 872 +15
Lines 12261 12462 +201
Branches 3221 3282 +61
=======================================
+ Hits 12068 12259 +191
- Misses 188 194 +6
- Partials 5 9 +4
... and 1 file with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found Additional details and impacted files@@ Coverage Diff @@
## main #2731 +/- ##
==========================================
- Coverage 98.42% 98.37% -0.06%
==========================================
Files 857 872 +15
Lines 12261 12462 +201
Branches 3183 3282 +99
==========================================
+ Hits 12068 12259 +191
- Misses 188 194 +6
- Partials 5 9 +4
... and 1 file with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will increase total bundle size by 28.27kB ⬆️
|
Bundle ReportChanges will increase total bundle size by 28.27kB ⬆️
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
JerrySentry
changed the title
| function setup(data) { | ||
| useRepoBackfillingStatus.mockReturnValue(data) | ||
|
|
||
| render(<BackfillBanners />) |
There was a problem hiding this comment.
Can you remove the render function from the setup function, and call it directly after the setup function in each of the it blocks
| componentsMeasurementsActive: false, | ||
| isRepoBackfilling: false, | ||
| }) | ||
| expect(screen.getByText(/TriggerSyncBanner/)).toBeInTheDocument() |
There was a problem hiding this comment.
Can you pull out the screen.getByText and assign to a const and than use that const in the expect call.
There was a problem hiding this comment.
@nicholas-codecov any reason for this in particular?
There was a problem hiding this comment.
rather, why is that necessary/good coding practice?
There was a problem hiding this comment.
More or less just consistency throughout the app ... also looks a bit cleaner imo, and since we're introducing new tests I'd prefer to "build for tomorrow" rather than today and try to keep things tidy than when I found them if able
| componentsMeasurementsActive: true, | ||
| isRepoBackfilling: true, | ||
| }) | ||
| expect(screen.getByText(/SyncingBanner/)).toBeInTheDocument() |
| <MemoryRouter initialEntries={['/gh/codecov/gazebo/components']}> | ||
| <Route path="/:provider/:owner/:repo/components" exact={true}> | ||
| <QueryClientProvider client={queryClient}> | ||
| <SyncingBanner /> | ||
| </QueryClientProvider> | ||
| </Route> | ||
| </MemoryRouter> |
There was a problem hiding this comment.
Can you pull the MemoryRouter, Route, and QueryClientProvider into a wrapper component, and than move the render into the it block
| beforeEach(() => { | ||
| setup() | ||
| }) |
There was a problem hiding this comment.
Can you remove the beforeEach and just call setup in the it block before rendering
| name={name} | ||
| /> | ||
| ), | ||
| delete: true ? ( |
There was a problem hiding this comment.
Shouldn't delete only appear for admins?
|
|
||
| describe('TableSparkline', () => { | ||
| function setup(props: TableSparklineProps) { | ||
| render(<TableSparkline {...props} />) |
There was a problem hiding this comment.
Can you move the render call into the it blocks to bring this test up to date.
| jest.mock('services/repo/useRepoFlags') | ||
| jest.mock('services/repo') | ||
|
|
||
| jest.mock('services/navigation', () => ({ | ||
| ...jest.requireActual('services/navigation'), | ||
| useLocationParams: jest.fn(), | ||
| })) | ||
| jest.mock('react-router-dom', () => ({ | ||
| ...jest.requireActual('react-router-dom'), | ||
| useParams: jest.fn(), | ||
| })) |
There was a problem hiding this comment.
Can we remove these mocks, and replace with a proper router wrapper, and msw mocks
| @@ -86,6 +86,12 @@ export function useStaticNavLinks() { | |||
| isExternalLink: true, | |||
| openNewTab: true, | |||
| }, | |||
| deployingComponentsSupport: { | |||
| text: 'Enabling Flags on Self Hosted', | |||
There was a problem hiding this comment.
Should this line here have Components instead of Flags
| @@ -278,6 +284,12 @@ export function useStaticNavLinks() { | |||
| isExternalLink: true, | |||
| openNewTab: true, | |||
| }, | |||
| componentsFeedback: { | |||
| text: 'New repo set up feedback', | |||
There was a problem hiding this comment.
Should this text be updated for components?
|
|
||
| describe('BackfillBanner', () => { | ||
| function setup(data) { | ||
| useRepoBackfillingStatus.mockReturnValue(data) |
There was a problem hiding this comment.
m: Is this hook going to be replaced by one specific for components? If so it's fine, just note down that this needs to be updated to an MSW mock when the switch is made, if not than we should update now.
There was a problem hiding this comment.
Yes this will be replaced by useComponentsBackfilled which calls the BackfillComponentMemberships query, will add a TODO here to replace with MSW when that switch is made in the coming step.
| jest.mock('services/user') | ||
|
|
||
| jest.mock('services/repo/useActivateFlagMeasurements') |
There was a problem hiding this comment.
m: These should probably be updated to MSW mocks as well
| jest.mock('react-router-dom', () => ({ | ||
| ...jest.requireActual('react-router-dom'), // import and retain the original functionalities | ||
| useParams: jest.fn(() => {}), | ||
| })) |
There was a problem hiding this comment.
l: If we're using a MemoryRouter and Route we shouldn't need this mock.
| const backfill = screen.getByTestId('backfill-task') | ||
| await user.click(backfill) | ||
|
|
||
| await waitFor(() => expect(mutate).toHaveBeenCalled()) |
There was a problem hiding this comment.
l: Can you add in another check here with .toHaveBeenCalledWith({ ... }) so we can make sure the correct args are being passed.
| '/gh/codecov/gazebo?flags%5B0%5D=flagA' | ||
| ) | ||
|
|
||
| user.click(flagA) |
Description
codecov/engineering-team#1370
Adds a placeholder Components tab under repo page, this for now will be showing Flag data. From there we will step by step update this to fully show Component analytics data.
See list of tasks here: codecov/engineering-team#234
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.