[RW-9608][risk=no] Added polling for app status #7517
Conversation
…sByDisplayGroup working correctly.
| }); | ||
| }); | ||
|
|
||
| // these tests assume that there are no User GKE Apps |
There was a problem hiding this comment.
These tests are indirectly testing the getAppsByDisplayGroup functionality, and are therefore unnecessary. Since these tests also rely on store values, parallel tests make them impractical to test effectively.
| const { runtime } = useStore(runtimeStore); | ||
| const { config } = useStore(serverConfigStore); | ||
| // all GKE apps (not Jupyter) | ||
| const { userApps } = useStore(userAppsStore); |
There was a problem hiding this comment.
Do we have an ontology here? For example, Set(allUserApps) = Set(gkeUserApps) + Set(nonGkeUserApps)?
There was a problem hiding this comment.
the terms "user apps" and "GKE apps" are equivalent. Jupyter uses (GCE or DataProc) "runtimes" instead of (GKE) "user apps"
User Environments is the set of both (user apps + runtimes)
| }, | ||
| }; | ||
|
|
||
| describe('User Apps Helper functions', () => { |
There was a problem hiding this comment.
I like these. I get a sense as to how the utilities work.
ui/src/app/utils/user-apps-utils.tsx
Outdated
| export const createUserApp = (namespace, config: CreateAppRequest) => { | ||
| return appsApi() | ||
| .createApp(namespace, config) | ||
| .then(() => { | ||
| maybeStartPollingForUserApps(namespace); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Love how clean and clear these are. I prefer the syntax that omits the block whenever I don't need it because it calls to attention the cases where I do need it.
| export const createUserApp = (namespace, config: CreateAppRequest) => { | |
| return appsApi() | |
| .createApp(namespace, config) | |
| .then(() => { | |
| maybeStartPollingForUserApps(namespace); | |
| }); | |
| }; | |
| export const createUserApp = (namespace, config: CreateAppRequest) => | |
| appsApi() | |
| .createApp(namespace, config) | |
| .then(() => maybeStartPollingForUserApps(namespace)); |
There was a problem hiding this comment.
I like the look of this too. Thanks for pointing it out.
| }); | ||
| }); | ||
|
|
||
| // these tests assume that there are no User GKE Apps |
| expect(findAvailableApps(wrapper, false).exists()).toBeFalsy(); | ||
| }); | ||
|
|
||
| it('should pause a running RStudio app', async () => { |
There was a problem hiding this comment.
These are not like the "getAppsByDisplayGroup" tests above - are there equivalents for these tests elsewhere? @evrii
| .listAppsInWorkspace(namespace) | ||
| .then((listAppsResponse) => { | ||
| userAppsStore.set({ userApps: listAppsResponse, updating: false }); | ||
| if (doUserAppsRequireUpdates()) { |
There was a problem hiding this comment.
this polling is easy to understand
| }); | ||
| }; | ||
|
|
||
| export const createUserApp = (namespace, config: CreateAppRequest) => |
| } | ||
|
|
||
| export interface UserAppsStore { | ||
| updating?: boolean; |
There was a problem hiding this comment.
should these be non-optional? we should always have these values if the store is populated, right?
| ); | ||
| await userAppsUtils.maybeStartPollingForUserApps('fakeNameSpace'); | ||
|
|
||
| jest.advanceTimersByTime(20e3); |
There was a problem hiding this comment.
nit/thinking out loud ... would be nice to have a const to compare this against. Something like "appPollingInterval + 1e3" to show that it's clearly longer than that interval
Description:
PR checklist