-
Notifications
You must be signed in to change notification settings - Fork 5
Core 1268 review portal routing and tests #2784
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
Conversation
0689635 to
a3897b7
Compare
| "setupFiles": [ | ||
| "<rootDir>/test/setupFile.js" | ||
| ], | ||
| "maxWorkers": "60%", |
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.
This was to help some flaky tests succeed. I'm not sure whether it helped, but I think it did.
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.
Diffs were too many to show changes. :(
| function useLoading(name) { | ||
| const {pathname} = useLocation(); | ||
|
|
||
| return React.useCallback( |
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 removed this somewhat complicated handler and replaced with simply the LoadingPlaceholder, which is how it's done elsewhere in the code. The error handling was not relevant to the ways it could actually fail.
| return <RouteAsPortalOrNot />; | ||
| } | ||
|
|
||
| export function isNoDataPage(dir: string) { |
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.
Handling these in both portal and non-portal routes was the main issue I had to fix.
|
|
||
| if (isFlex) { | ||
| return <FlexPage data={data} />; | ||
| if (data?.body) { |
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.
Just making this all follow the same short-circuit pattern.
| } from '~/components/book-selector/book-selector'; | ||
| import {useAfterSubmit} from '~/components/book-selector/after-form-submit'; | ||
| import {MemoryRouter} from 'react-router-dom'; | ||
| import MemoryRouter from '~/../../test/helpers/future-memory-router'; |
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 finally went through and replaced all of these. There are a bunch. The standard memory router gives a warning, making a lot of tests noisy.
| await screen.findByText('What is your question about?'); | ||
| expect(screen.queryAllByRole('navigation')).toHaveLength(0); | ||
| }); | ||
| it('handles piTracker ', async () => { |
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.
This is tested in use-link-handler, and the test here is flaky.
|
|
||
| return <div>-{loc.pathname}-</div>; | ||
| } | ||
| function mockBrowserInitialEntriesWithLocation(entries: string[]) { |
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.
Removed a bunch of repetitive calls in tests.
CORE-1268