You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR refactors <RightPanel> so that Page component tests are no longer also testing the RightPanel component. A separate ticket will further explore possible changes to useSocket based on the findings in the discussion of this PR.
Previous WIP notes
This PR currently includes some changes that resolved the console errors previously seen in the test file. I don't see this PR as it stands now as a comprehensive approach to resolving this issue, but want to document what I've found so far
Prior to these changes:
running "renders a selected vehicle" and "on mobile, allows you to toggle to the map view and back again" one at a time: both succeeded
running all SearchPage.test.tsx at once with "renders a selected vehicle" before "on mobile, allows you to toggle to the map view and back again": both tests succeed, but console errors logged
running all SearchPage.test.tsx at once with "renders a selected vehicle" after "on mobile, allows you to toggle to the map view and back again": both tests succeed, no console errors logged.
Isolating the issue:
removing the selectedVehicleOrGhost from state in "renders a selected vehicle": both tests succeed, no console errors logged
removing async/await from "on mobile, allows you to toggle to the map view and back again": both tests succeed, no console errors
Mocking out <RightPanel>: both tests succeed, no console errors logged
After performing the above tests, I wondered if something about the call to react-test-renderer.create() in "renders a selected vehicle" wasn't being cleaned up properly before running the next test.
Switched to use react-testing-library renderer instead to avoid errors due to inconsistent renderers within the same file
Mocked out additional API calls used in the VPP to resolve Error: Error: connect ECONNREFUSED 127.0.0.1:80 errors
most potentially concerning change : added cleanup function to useSocket to resolve Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
While those fixes within the test file resolved the console errors, it raises a few more questions:
Would moving <RightPanel> up a level so it is a sibling to these pages rather than a child help to keep these tests isolated?
is the useSocket hook not having a cleanup function a real problem, or a symptom of something else missing in the test? I suspect it is a real problem, but have doubts since it is an older area of the code. Though this kind of issue might not have user-facing impact
Is there an advantage I'm overlooking to using react-test-renderer for snapshots over react-testing-library render+asFragment`?
Summary coverage rate:
lines......: 94.0% (2463 of 2620 lines)
functions..: 73.0% (1034 of 1417 functions)
branches...: no data found
Files changed coverage rate: n/a
With regards to moving <RightPanel> up a level, at first I was undecided but now I'm in favor. My fear was that there could be pages where we don't want the right panel to appear, but then I remembered that currently you can access notifications from any page, and opening a notification entails being able to open the VPP. If that ever changes we'll need to make other significant changes to Skate, and independently rendering <RightPanel> in each separate existing page component is repeated code. So yeah, I'd say go for it, it will make the application code cleaner and it sounds like it will help with testing too.
👍 Thanks @lemald, I'll rework this PR to move <RightPanel>.
I suspsect that one possible user-facing added benefit of moving <RightPanel> up a level is that it the panel won't re-render when switching between modes. Right now:
open the VPP from search and open the block minischedule
navigate to the route ladder. VPP reset to vehicle status
I expect with this change, the VPP would stay on the block minischedule when switching between modes. Before making this change I can confirm with product in the ticket whether the existing behavior is intended.
👍 Thanks @lemald, I'll rework this PR to move <RightPanel>.
I suspsect that one possible user-facing added benefit of moving <RightPanel> up a level is that it the panel won't re-render when switching between modes. Right now:
1. open the VPP from search and open the block minischedule
2. navigate to the route ladder. VPP reset to vehicle status
I expect with this change, the VPP would stay on the block minischedule when switching between modes. Before making this change I can confirm with product in the ticket whether the existing behavior is intended.
Ah, yes, that's also a good point! I suspect you're correct about how it will work with the change, and it should be a nice (if small) UX improvement.
Summary coverage rate:
lines......: 94.0% (2463 of 2620 lines)
functions..: 73.0% (1034 of 1417 functions)
branches...: no data found
Files changed coverage rate: n/a
Summary coverage rate:
lines......: 94.0% (2463 of 2620 lines)
functions..: 73.0% (1034 of 1417 functions)
branches...: no data found
Files changed coverage rate: n/a
Summary coverage rate:
lines......: 94.0% (2463 of 2620 lines)
functions..: 73.0% (1034 of 1417 functions)
branches...: no data found
Files changed coverage rate: n/a
There may still need to be a cleanup added for this hook, but it isn't immediately causing issues with the tests and will be investigated as part of a separate ticket
Summary coverage rate:
lines......: 94.0% (2463 of 2620 lines)
functions..: 73.0% (1034 of 1417 functions)
branches...: no data found
Files changed coverage rate: n/a
Summary coverage rate:
lines......: 94.0% (2463 of 2620 lines)
functions..: 73.0% (1034 of 1417 functions)
branches...: no data found
Files changed coverage rate: n/a
Summary coverage rate:
lines......: 94.0% (2463 of 2620 lines)
functions..: 73.0% (1034 of 1417 functions)
branches...: no data found
Files changed coverage rate: n/a
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
KaylaBrady
force-pushed
the
kb-test-interdependency
branch
from
ticket: https://app.asana.com/0/1152340551558956/1203058157664665/f
This PR refactors
<RightPanel>so thatPagecomponent tests are no longer also testing theRightPanelcomponent. A separate ticket will further explore possible changes touseSocketbased on the findings in the discussion of this PR.Previous WIP notes
This PR currently includes some changes that resolved the console errors previously seen in the test file. I don't see this PR as it stands now as a comprehensive approach to resolving this issue, but want to document what I've found so farPrior to these changes:
SearchPage.test.tsxat once with "renders a selected vehicle" before "on mobile, allows you to toggle to the map view and back again": both tests succeed, but console errors loggedSearchPage.test.tsxat once with "renders a selected vehicle" after "on mobile, allows you to toggle to the map view and back again": both tests succeed, no console errors logged.Isolating the issue:
selectedVehicleOrGhostfrom state in "renders a selected vehicle": both tests succeed, no console errors loggedasync/awaitfrom "on mobile, allows you to toggle to the map view and back again": both tests succeed, no console errors<RightPanel>: both tests succeed, no console errors loggedAfter performing the above tests, I wondered if something about the call to
react-test-renderer.create()in "renders a selected vehicle" wasn't being cleaned up properly before running the next test.react-testing-libraryrenderer instead to avoid errors due to inconsistent renderers within the same fileError: Error: connect ECONNREFUSED 127.0.0.1:80errorsuseSocketto resolveWarning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.While those fixes within the test file resolved the console errors, it raises a few more questions:
<RightPanel>up a level so it is a sibling to these pages rather than a child help to keep these tests isolated?useSockethook not having a cleanup function a real problem, or a symptom of something else missing in the test? I suspect it is a real problem, but have doubts since it is an older area of the code. Though this kind of issue might not have user-facing impactreact-test-rendererfor snapshots overreact-testing-libraryrender+asFragment`?