Skip to content
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

[Tests] Amend typos and extraneous <Set /> generic typings in router tests #11894

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[Tests] Amend typos and extraneous <Set /> generic typings in router tests #11894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9971ea5
Amend test descriptions
Philzen Jan 10, 2025
8f82626
Fix type error by removing generic injected type (not needed since #1…
Philzen Jan 10, 2025
64a73a4
Merge branch 'main' into tests/fix-typos-and-set-typings
Tobbe Jan 17, 2025
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
2 changes: 1 addition & 1 deletion packages/router/src/__tests__/analyzeRoutes.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ test('Nested sets, and authentication logic', () => {
})
})

test('Give correct ids to root sets', () => {
test('gives correct ids to root sets', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think this was better the way it was.
Had this been an it-test instead I would have agreed with your change. But with test I think "test: Give correct..." reads better

(But I really wish all tests used it())

const Layout = ({ children }: LayoutProps) => <>{children}</>

const Routes = (
Expand Down
2 changes: 1 addition & 1 deletion packages/router/src/__tests__/route-validators.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('isValidRoute', () => {
)
})

it("does not throws if notFoundPage doesn't have a path", () => {
it("does not throw if notFoundPage doesn't have a path", () => {
// @ts-expect-error Its ok mate, we're checking the validator
const RouteToCheck = <Route name="bazinga" notfound page={() => <></>} />

Expand Down
14 changes: 3 additions & 11 deletions packages/router/src/__tests__/router.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,11 +925,7 @@ test('Private is an alias for Set private', async () => {
const TestRouter = () => (
<Router useAuth={mockUseAuth({ isAuthenticated: true })}>
<Route path="/" page={HomePage} name="home" />
<Private<PrivateLayoutProps>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With all the type change PRs reverted I think I want to keep this as it was. What's your thought on that, @Philzen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the end of the day, these are completely redundant – there is no more or less TS errors in these files with or without them.

But let me double check if it changes anything regarding auto-completion – although even then, they would be just noise in the test, as they don't serve any purpose concerning stuff that would be able to come up in a CI.

My thought is to find a use case where they are actually required and implement a test for that, whilst removing those generics where they aren't.

wrap={PrivateLayout}
unauthenticated="home"
theme="dark"
>
<Private wrap={PrivateLayout} unauthenticated="home" theme="dark">
<Route path="/private" page={PrivatePage} name="private" />
</Private>
</Router>
Expand Down Expand Up @@ -1228,11 +1224,7 @@ describe('Multiple nested private sets', () => {
const TestRouter = ({ useAuthMock }: { useAuthMock: UseAuth }) => (
<Router useAuth={useAuthMock}>
<Route path="/" page={HomePage} name="home" />
<PrivateSet<LevelLayoutProps>
unauthenticated="home"
level="1"
wrap={LevelLayout}
>
<PrivateSet unauthenticated="home" level="1" wrap={LevelLayout}>
<Route
path="/no-roles-assigned"
page={PrivateNoRolesAssigned}
Expand Down Expand Up @@ -1357,7 +1349,7 @@ describe('Multiple nested sets', () => {
const TestRouter = () => (
<Router>
<Route path="/" page={HomePage} name="home" />
<Set<DebugLayoutProps> level="1" theme="blue" wrap={DebugLayout}>
<Set level="1" theme="blue" wrap={DebugLayout}>
<Route path="/level1" page={Page} name="level1" />
<Set level="2" theme="red" otherProp="bazinga">
<Route path="/level2" page={Page} name="level2" />
Expand Down
Loading