feat: Test Run / Navigator polling updates for collection jobs #1125
Conversation
|
Decided at the meeting today to continue on with splitting up this page such that only the updating parts care about updates so it does less "whole page refresh" feeling tasks |
There was a problem hiding this comment.
@gnarf changes look good to me in the best case scenarios (admin, tester, vendors, etc). But I have a consistent crash when viewing anything in the test queue as an anonymous user. That seems to be because of a valid assumption here that a testPlanRun variable would always be populated. But the anon user operates on `testPlanReport.
I've long since thought this file should be severely refactored (with myself being the culprit from some time back) and there's certainly room for breaking more things out here that shouldn't be included
gnarf
force-pushed
the
test-report-updates
branch
from
8e11517
to
255a245
Compare
| actions: {} | ||
| }); | ||
|
|
||
| export const Provider = ({ children, testPlanRun }) => { |
There was a problem hiding this comment.
I like this way of sharing the polling across multiple components!
| } else { | ||
| stopPolling(); | ||
| } | ||
| setProviderValue({ state: { collectionJob }, actions: {} }); |
There was a problem hiding this comment.
| setProviderValue({ state: { collectionJob }, actions: {} }); | |
| setProviderValue({ state: { collectionJob }, actions: {} }); | |
| return () => { | |
| stopPolling(); | |
| } |
Could be good to return a cleanup function in the event of a component unmount
There was a problem hiding this comment.
Not sure what this means exactly. Would this to be to enable the page to stop polling when the components on the page don't need it anymore? Seems like this case wouldn't be necessary, a minor save for what is unlikely to be a permanent situation (most up to date CollectionJob info for the current TestPlanRun is what this provider is providing) The useContext mechanism should automatically unmount/disconnect from the updates when it does.
There was a problem hiding this comment.
This is just a cleanup function to ensure that the polling is stopped even if the context is unmounted at an unexpected time. My understanding is that this is best practice for any useEffect with an async side effect.
There was a problem hiding this comment.
Tested this and the rendering is so much smoother now. Major improvement! At some point it might be cool to try and abstract that Context even further to make a general polling provider. I can imagine that could be useful elsewhere in the future. Thanks for going the extra mile on this one!
gnarf
force-pushed
the
test-report-updates
branch
from
255a245
to
08d9e3c
Compare
I feel like if we end up needing it again, I could pretty easily do a |
| @@ -111,7 +111,33 @@ describe('smoke test', () => { | |||
| const h1Handle = await page.waitForSelector('h1'); | |||
| const h1Text = await h1Handle.evaluate(h1 => h1.innerText); | |||
| expect(h1Text).toBe('Data Management'); | |||
| }) | |||
| }), | |||
| getPage( | |||
There was a problem hiding this comment.
Thanks for updating the tests to account for this
Includes the following changes: **Features and Fixes** * #1125 * #1135 * #1144 * #1146 * #1150 * #1152 * #1153 * #1160 **Infrastructure changes** * #969 * #1151 * #1148 --------- Co-authored-by: alflennik <[email protected]> Co-authored-by: Mx Corey Frang <[email protected]> Co-authored-by: cypress evelyn masso <[email protected]> Co-authored-by: Stalgia Grigg <[email protected]>
The following improvements should be made to the test run page.