Skip to content

Add Tests for Custom Hooks (1/6) #12486

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.

Sign up for GitHub

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

Jump to bottom
Closed
7 of 14 tasks
Parkreiner opened this issue Mar 9, 2024 · 2 comments
Closed
7 of 14 tasks

Add Tests for Custom Hooks (1/6) #12486

Parkreiner opened this issue Mar 9, 2024 · 2 comments
Assignees
@Parkreiner
Labels
site Area: frontend dashboard

Comments

@Parkreiner
Copy link
Member

Parkreiner commented Mar 9, 2024 •
edited
Loading

Split off from #11421
Following @sreya 's lead by splitting up the custom hook issue into more manageable chunks.

I feel like I've done the most custom hook testing out of anybody in the company, so I'm happy to take this chunk, which is likely going to be the most complicated set

Global reusable hooks

Hook name Needs Comments
useDebouncedFunction Touch up One of the first custom hooks tests I ever wrote
useDebouncedValue Touch up One of the first custom hooks tests I ever wrote
useCustomEvent Touch up In a great spot; some assertions could be simplified
useEffectEvent Touch up Not much to it, but it's become ingrained in many parts of the codebase already
useClassName Tests My main worry with this hook is that it's adding CSS directly to the HTML from within the render logic (it's using Vanilla Emotion, and not the React-specific flavor that uses useInsertionEffect). Not sure if that also means that styles can never be cleaned up
useClickable Tests Could probably have some TypeScript type definitions cleaned up too (sorry)
useClickableTableRow Redesign or deletion This is causing more accessibility problems than it's solving. See #12248
useClipboard Tests Especially important, since we had a regression around this
usePaginatedQuery Re-review Might be good to go; there are a lot of tests for this already. By far the most complicated hook in this group
usePagination Tests Feels like it's right at the boundary between a custom hook and a utility function; might be better to remove the hook behavior
useSearchParamKey Tests Previously useTab; got beefed up and generalized
useTime Tests Hook just got added. Wondering if we could start using it to get rid of (or at least minimize) some of the time-based side effects we're baking directly into render logic...
useWorkspaceBuildLogs Tests Uses websockets under the hood – not sure how tough that will be to deal with, considering that MSW apparently hasn't added support yet

Other hooks

I'm also including this other hook with this group, because it's used in a few different spots, and I think updating it could require a bit of in-depth knowledge of React rendering behavior:

Hook name Filepath Needs Comments
useProxyLatency ./src/contexts/useProxyLatency Bug fixes, possible test case touch-ups There's some test coverage here already, but from my quick review, there are some logic bugs in the hook that need to fixed

And throwing this one simply because it's already done, and I don't know where else to put it:

Hook name Filepath Needs Comments
useWorkspaceDuplication ./src/pages/CreateWorkspacePage/ Has tests Might be worth moving this to the hooks folder?

Task list

  • useDebouncedFunction
  • useDebouncedValue
  • useCustomEvent
  • useClassName
  • useClickable
  • useClickableTableRow
  • useClipboard
  • usePagination
  • usePaginatedQuery
  • useSearchParamKey
  • useTime
  • useWorkspaceBuildLogs
  • useProxyLatency
  • useWorkspaceDuplication

Notes

  • I'm going to be a bit busy with Backstage launch until next Friday, so I probably won't be able to start any other hooks until then. People are free to take any of these hooks, but I would appreciate being tagged on the PRs
    • Ben and I were talking the other day about doing taking a week-long breather at some point after launch to do some tests and bug fixes in core (though if we get a bunch of Backstage feedback, that might have to wait)
@Parkreiner Parkreiner added the site Area: frontend dashboard label Mar 9, 2024
@Parkreiner Parkreiner added this to the FE Code Stability milestone Mar 9, 2024
@Parkreiner Parkreiner self-assigned this Mar 9, 2024
@Parkreiner Parkreiner changed the title Custom Hook Test Coverage (1/4) Add Tests for Custom Hook (1/4) Mar 9, 2024
@Parkreiner Parkreiner changed the title Add Tests for Custom Hook (1/4) Add Tests for Custom Hooks (1/4) Mar 9, 2024
@Parkreiner Parkreiner changed the title Add Tests for Custom Hooks (1/4) Add Tests for Custom Hooks (1/6) Mar 9, 2024
@Parkreiner
Copy link
Member Author

With the changes made in #12597, it looks we have a new issue: #12598
Un-checking the bullet for usePaginatedQuery

@bpmct bpmct removed this from the Code Stability milestone Apr 16, 2024
@bpmct
Copy link
Member

bpmct commented May 31, 2024

We can reopen when we are ready to tackle, but want to keep the tracker clean for community members

@bpmct bpmct closed this as not planned Won't fix, can't repro, duplicate, stale May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Parkreiner Parkreiner

Labels
site Area: frontend dashboard
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bpmct @Parkreiner