From ab6539498273f22f4b206013a2bae1aeae41d403 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 20 Aug 2024 15:45:47 -0500 Subject: [PATCH 01/11] first pass at instance list polling --- app/pages/project/instances/InstancesPage.tsx | 65 +++++++++++++++++-- app/util/array.spec.tsx | 13 +++- app/util/array.ts | 10 +++ 3 files changed, 83 insertions(+), 5 deletions(-) diff --git a/app/pages/project/instances/InstancesPage.tsx b/app/pages/project/instances/InstancesPage.tsx index 073807b0db..9ccb43e339 100644 --- a/app/pages/project/instances/InstancesPage.tsx +++ b/app/pages/project/instances/InstancesPage.tsx @@ -7,11 +7,12 @@ */ import { createColumnHelper } from '@tanstack/react-table' import { filesize } from 'filesize' -import { useMemo } from 'react' +import { useMemo, useRef } from 'react' import { useNavigate, type LoaderFunctionArgs } from 'react-router-dom' import { apiQueryClient, + instanceTransitioning, useApiQueryClient, usePrefetchedApiQuery, type Instance, @@ -29,7 +30,9 @@ import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable' import { CreateLink } from '~/ui/lib/CreateButton' import { EmptyMessage } from '~/ui/lib/EmptyMessage' import { PageHeader, PageTitle } from '~/ui/lib/PageHeader' +import { Spinner } from '~/ui/lib/Spinner' import { TableActions } from '~/ui/lib/Table' +import { isSetEqual } from '~/util/array' import { docLinks } from '~/util/links' import { pb } from '~/util/path-builder' @@ -55,6 +58,8 @@ InstancesPage.loader = async ({ params }: LoaderFunctionArgs) => { return null } +const POLLING_TIMEOUT = 5_000 // 30 seconds + export function InstancesPage() { const { project } = useProjectSelector() @@ -66,9 +71,56 @@ export function InstancesPage() { { onSuccess: refetchInstances, onDelete: refetchInstances } ) - const { data: instances } = usePrefetchedApiQuery('instanceList', { - query: { project, limit: PAGE_SIZE }, - }) + // this is a whole thing. sit down. + const transitioningInstances = useRef>(new Set()) + const pollingStart = useRef(Date.now()) + + const { data: instances, isFetching } = usePrefetchedApiQuery( + 'instanceList', + { + query: { project, limit: PAGE_SIZE }, + }, + { + refetchInterval({ state }) { + const currTransitioning = transitioningInstances.current + const nextTransitioning = new Set( + // data will never actually be undefined because of the prefetch but whatever + state.data?.items.filter(instanceTransitioning).map((i) => i.id) || [] + ) + + // always update. we don't have to worry about doing this in all the branches below. + transitioningInstances.current = nextTransitioning + + // if no instances are transitioning, stop polling + if (nextTransitioning.size === 0) { + pollingStart.current = null + return false + } + + // if we have new transitioning instances, we always restart polling + // regardless of whether we were polling before + if (!isSetEqual(currTransitioning, nextTransitioning)) { + pollingStart.current = Date.now() + return 1000 + } + + // if the set hasn't changed, we still poll unless we've hit the timeout + if ( + pollingStart.current !== null && + Date.now() - pollingStart.current < POLLING_TIMEOUT + ) { + // don't update pollingStart: we need the timer to keep running down + return 1000 + } + + // at this point we know: + // - the set of transitioning instances hasn't changed + // - we are no longer within the polling window + pollingStart.current = null + return false + }, + } + ) const navigate = useNavigate() useQuickActions( @@ -145,6 +197,11 @@ export function InstancesPage() { /> + {isFetching && ( +
+ +
+ )} New Instance
diff --git a/app/util/array.spec.tsx b/app/util/array.spec.tsx index ad46d4c964..7d88156df4 100644 --- a/app/util/array.spec.tsx +++ b/app/util/array.spec.tsx @@ -8,7 +8,7 @@ import { type ReactElement } from 'react' import { expect, test } from 'vitest' -import { groupBy, intersperse } from './array' +import { groupBy, intersperse, isSetEqual } from './array' test('groupBy', () => { expect( @@ -61,3 +61,14 @@ test('intersperse', () => { expect(result.map(getText)).toEqual(['a', ',', 'b', ',', 'or', 'c']) expect(result.map(getKey)).toEqual(['a', 'sep-1', 'b', 'sep-2', 'conj', 'c']) }) + +test('isSetEqual', () => { + expect(isSetEqual(new Set(), new Set())).toBe(true) + expect(isSetEqual(new Set(['a', 'b', 'c']), new Set(['a', 'b', 'c']))).toBe(true) + + expect(isSetEqual(new Set(['a']), new Set(['b']))).toBe(false) + expect(isSetEqual(new Set(['a']), new Set(['a', 'b']))).toBe(false) + expect(isSetEqual(new Set(['a', 'b']), new Set(['a']))).toBe(false) + + expect(isSetEqual(new Set([{}]), new Set([{}]))).toBe(false) +}) diff --git a/app/util/array.ts b/app/util/array.ts index 2b28eed4b5..e2048e31bb 100644 --- a/app/util/array.ts +++ b/app/util/array.ts @@ -38,3 +38,13 @@ export function intersperse( return [sep0, item] }) } + +export function isSetEqual(a: Set, b: Set): boolean { + if (a.size !== b.size) return false + for (const item of a) { + if (!b.has(item)) { + return false + } + } + return true +} From b5a6a04825f012d1070b345b3cd75d541caebb90 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 20 Aug 2024 16:05:27 -0500 Subject: [PATCH 02/11] do it cleaner --- app/pages/project/instances/InstancesPage.tsx | 57 +++++++++---------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/app/pages/project/instances/InstancesPage.tsx b/app/pages/project/instances/InstancesPage.tsx index 9ccb43e339..1b175360f6 100644 --- a/app/pages/project/instances/InstancesPage.tsx +++ b/app/pages/project/instances/InstancesPage.tsx @@ -72,52 +72,47 @@ export function InstancesPage() { ) // this is a whole thing. sit down. + + // We initialize this set as empty because we don't have the instances on hand + // yet. This is fine because the first fetch will recognize the presence of + // any transitioning instances as a change in state and initiate polling const transitioningInstances = useRef>(new Set()) - const pollingStart = useRef(Date.now()) + const pollingStartTime = useRef(Date.now()) const { data: instances, isFetching } = usePrefetchedApiQuery( 'instanceList', + { query: { project, limit: PAGE_SIZE } }, { - query: { project, limit: PAGE_SIZE }, - }, - { - refetchInterval({ state }) { - const currTransitioning = transitioningInstances.current + // The point of all this is to poll for a certain amount of time after + // some instance in the current page is transitioning. For example, if + // you manually stop an instance, its state will change to `stopping`, + // which will cause this logic to start polling the list until it lands + // in `stopped`, which is not a transitional state. We want polling to + // eventually time out so we're not polling forever if an instance is + // stuck in starting or whatever. + refetchInterval({ state: { data } }) { + // these are sets of instance UUIDs + const prevTransitioning = transitioningInstances.current const nextTransitioning = new Set( // data will never actually be undefined because of the prefetch but whatever - state.data?.items.filter(instanceTransitioning).map((i) => i.id) || [] + data?.items.filter(instanceTransitioning).map((i) => i.id) || [] ) // always update. we don't have to worry about doing this in all the branches below. transitioningInstances.current = nextTransitioning - // if no instances are transitioning, stop polling - if (nextTransitioning.size === 0) { - pollingStart.current = null - return false - } - - // if we have new transitioning instances, we always restart polling - // regardless of whether we were polling before - if (!isSetEqual(currTransitioning, nextTransitioning)) { - pollingStart.current = Date.now() - return 1000 - } + // if no instances are transitioning, do not poll + if (nextTransitioning.size === 0) return false - // if the set hasn't changed, we still poll unless we've hit the timeout - if ( - pollingStart.current !== null && - Date.now() - pollingStart.current < POLLING_TIMEOUT - ) { - // don't update pollingStart: we need the timer to keep running down - return 1000 + // if the set of transitioning instances hasn't changed, we only poll if we haven't hit the timeout + if (isSetEqual(prevTransitioning, nextTransitioning)) { + const elapsed = Date.now() - pollingStartTime.current + return elapsed < POLLING_TIMEOUT ? 1000 : false } - // at this point we know: - // - the set of transitioning instances hasn't changed - // - we are no longer within the polling window - pollingStart.current = null - return false + // if we have new transitioning instances, always poll and restart the window + pollingStartTime.current = Date.now() + return 1000 }, } ) From c4c41c39870e579d676a173ad313e740e49363fd Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 20 Aug 2024 16:22:14 -0500 Subject: [PATCH 03/11] add instance state to key --- app/pages/project/instances/InstancesPage.tsx | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/app/pages/project/instances/InstancesPage.tsx b/app/pages/project/instances/InstancesPage.tsx index 1b175360f6..2849e06529 100644 --- a/app/pages/project/instances/InstancesPage.tsx +++ b/app/pages/project/instances/InstancesPage.tsx @@ -30,7 +30,6 @@ import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable' import { CreateLink } from '~/ui/lib/CreateButton' import { EmptyMessage } from '~/ui/lib/EmptyMessage' import { PageHeader, PageTitle } from '~/ui/lib/PageHeader' -import { Spinner } from '~/ui/lib/Spinner' import { TableActions } from '~/ui/lib/Table' import { isSetEqual } from '~/util/array' import { docLinks } from '~/util/links' @@ -79,7 +78,7 @@ export function InstancesPage() { const transitioningInstances = useRef>(new Set()) const pollingStartTime = useRef(Date.now()) - const { data: instances, isFetching } = usePrefetchedApiQuery( + const { data: instances } = usePrefetchedApiQuery( 'instanceList', { query: { project, limit: PAGE_SIZE } }, { @@ -91,11 +90,17 @@ export function InstancesPage() { // eventually time out so we're not polling forever if an instance is // stuck in starting or whatever. refetchInterval({ state: { data } }) { - // these are sets of instance UUIDs const prevTransitioning = transitioningInstances.current const nextTransitioning = new Set( - // data will never actually be undefined because of the prefetch but whatever - data?.items.filter(instanceTransitioning).map((i) => i.id) || [] + // Data will never actually be undefined because of the prefetch but whatever + (data?.items || []) + .filter(instanceTransitioning) + // These are strings of instance ID + current state. This is done because + // of the case where an instance is stuck in starting (for example), polling + // times out, and then you manually stop it. Without putting the state in the + // the key, that stop action would not be registered as a change in the set + // of transitioning instances. + .map((i) => i.id + '-' + i.runState) ) // always update. we don't have to worry about doing this in all the branches below. @@ -192,11 +197,6 @@ export function InstancesPage() { /> - {isFetching && ( -
- -
- )} New Instance
From 1eb7244f5fa637be55d21497d25ffa1e36feed22 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 20 Aug 2024 16:33:04 -0500 Subject: [PATCH 04/11] poll for 30 seconds on a 3 second interval --- app/pages/project/instances/InstancesPage.tsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/pages/project/instances/InstancesPage.tsx b/app/pages/project/instances/InstancesPage.tsx index 2849e06529..f54dc4986e 100644 --- a/app/pages/project/instances/InstancesPage.tsx +++ b/app/pages/project/instances/InstancesPage.tsx @@ -57,7 +57,9 @@ InstancesPage.loader = async ({ params }: LoaderFunctionArgs) => { return null } -const POLLING_TIMEOUT = 5_000 // 30 seconds +const sec = 1000 // ms, obviously +const POLL_TIMEOUT = 30 * sec +const POLL_INTERVAL = 3 * sec export function InstancesPage() { const { project } = useProjectSelector() @@ -100,7 +102,7 @@ export function InstancesPage() { // times out, and then you manually stop it. Without putting the state in the // the key, that stop action would not be registered as a change in the set // of transitioning instances. - .map((i) => i.id + '-' + i.runState) + .map((i) => i.id + '|' + i.runState) ) // always update. we don't have to worry about doing this in all the branches below. @@ -112,12 +114,12 @@ export function InstancesPage() { // if the set of transitioning instances hasn't changed, we only poll if we haven't hit the timeout if (isSetEqual(prevTransitioning, nextTransitioning)) { const elapsed = Date.now() - pollingStartTime.current - return elapsed < POLLING_TIMEOUT ? 1000 : false + return elapsed < POLL_TIMEOUT ? POLL_INTERVAL : false } // if we have new transitioning instances, always poll and restart the window pollingStartTime.current = Date.now() - return 1000 + return POLL_INTERVAL }, } ) From 57173bc13e852de3e09b9c51d2d8924e75ca424a Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 23 Aug 2024 01:45:38 -0500 Subject: [PATCH 05/11] rework logic, use set diff instead of !setEq to trigger clock restart --- app/pages/project/instances/InstancesPage.tsx | 25 ++++++++++++------- app/util/array.spec.tsx | 11 +++++++- app/util/array.ts | 5 ++++ 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/app/pages/project/instances/InstancesPage.tsx b/app/pages/project/instances/InstancesPage.tsx index f54dc4986e..b639f414ac 100644 --- a/app/pages/project/instances/InstancesPage.tsx +++ b/app/pages/project/instances/InstancesPage.tsx @@ -31,7 +31,7 @@ import { CreateLink } from '~/ui/lib/CreateButton' import { EmptyMessage } from '~/ui/lib/EmptyMessage' import { PageHeader, PageTitle } from '~/ui/lib/PageHeader' import { TableActions } from '~/ui/lib/Table' -import { isSetEqual } from '~/util/array' +import { setDiff } from '~/util/array' import { docLinks } from '~/util/links' import { pb } from '~/util/path-builder' @@ -108,18 +108,25 @@ export function InstancesPage() { // always update. we don't have to worry about doing this in all the branches below. transitioningInstances.current = nextTransitioning - // if no instances are transitioning, do not poll - if (nextTransitioning.size === 0) return false + // We use this setDiff logic instead of just checking whether the set + // has changed because if you have two transitioning instances and + // one stops transitioning, then that's a change in the set, but you + // shouldn't start polling because of it! What you want to look for is + // new transitioning instances. + const anyTransitioning = nextTransitioning.size > 0 + const anyNewTransitioning = setDiff(nextTransitioning, prevTransitioning).size > 0 - // if the set of transitioning instances hasn't changed, we only poll if we haven't hit the timeout - if (isSetEqual(prevTransitioning, nextTransitioning)) { + if (anyTransitioning) { + // if we have new transitioning instances, restart the timeout clock + if (anyNewTransitioning) pollingStartTime.current = Date.now() + + // important that elapsed is calculated *after* bumping start time const elapsed = Date.now() - pollingStartTime.current - return elapsed < POLL_TIMEOUT ? POLL_INTERVAL : false + if (elapsed < POLL_TIMEOUT) return POLL_INTERVAL + // otherwise fall through to false below } - // if we have new transitioning instances, always poll and restart the window - pollingStartTime.current = Date.now() - return POLL_INTERVAL + return false }, } ) diff --git a/app/util/array.spec.tsx b/app/util/array.spec.tsx index 7d88156df4..f1b12bacf4 100644 --- a/app/util/array.spec.tsx +++ b/app/util/array.spec.tsx @@ -8,7 +8,7 @@ import { type ReactElement } from 'react' import { expect, test } from 'vitest' -import { groupBy, intersperse, isSetEqual } from './array' +import { groupBy, intersperse, isSetEqual, setDiff } from './array' test('groupBy', () => { expect( @@ -72,3 +72,12 @@ test('isSetEqual', () => { expect(isSetEqual(new Set([{}]), new Set([{}]))).toBe(false) }) + +test.only('setDiff', () => { + expect(setDiff(new Set(), new Set())).toEqual(new Set()) + expect(setDiff(new Set(['a']), new Set())).toEqual(new Set(['a'])) + expect(setDiff(new Set(), new Set(['a']))).toEqual(new Set()) + expect(setDiff(new Set(['b', 'a', 'c']), new Set(['b', 'd']))).toEqual( + new Set(['a', 'c']) + ) +}) diff --git a/app/util/array.ts b/app/util/array.ts index e2048e31bb..74d8b18dc0 100644 --- a/app/util/array.ts +++ b/app/util/array.ts @@ -48,3 +48,8 @@ export function isSetEqual(a: Set, b: Set): boolean { } return true } + +/** Set `a - b` */ +export function setDiff(a: Set, b: Set): Set { + return new Set([...a].filter((x) => !b.has(x))) +} From b6078ac797984d9b80919bcbf85f27072cced2d0 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 23 Aug 2024 10:43:49 -0500 Subject: [PATCH 06/11] add updated time. poll fast vs. slow, not fast vs. not at all --- app/pages/project/instances/InstancesPage.tsx | 67 ++++++++++--------- .../instances/instance/InstancePage.tsx | 2 +- app/ui/lib/Table.tsx | 2 +- 3 files changed, 39 insertions(+), 32 deletions(-) diff --git a/app/pages/project/instances/InstancesPage.tsx b/app/pages/project/instances/InstancesPage.tsx index b639f414ac..6c8a9773c1 100644 --- a/app/pages/project/instances/InstancesPage.tsx +++ b/app/pages/project/instances/InstancesPage.tsx @@ -31,7 +31,9 @@ import { CreateLink } from '~/ui/lib/CreateButton' import { EmptyMessage } from '~/ui/lib/EmptyMessage' import { PageHeader, PageTitle } from '~/ui/lib/PageHeader' import { TableActions } from '~/ui/lib/Table' +import { Tooltip } from '~/ui/lib/Tooltip' import { setDiff } from '~/util/array' +import { toLocaleTimeString } from '~/util/date' import { docLinks } from '~/util/links' import { pb } from '~/util/path-builder' @@ -58,8 +60,9 @@ InstancesPage.loader = async ({ params }: LoaderFunctionArgs) => { } const sec = 1000 // ms, obviously -const POLL_TIMEOUT = 30 * sec -const POLL_INTERVAL = 3 * sec +const POLL_FAST_TIMEOUT = 30 * sec +const POLL_INTERVAL_FAST = 3 * sec +const POLL_INTERVAL_SLOW = 60 * sec export function InstancesPage() { const { project } = useProjectSelector() @@ -80,17 +83,17 @@ export function InstancesPage() { const transitioningInstances = useRef>(new Set()) const pollingStartTime = useRef(Date.now()) - const { data: instances } = usePrefetchedApiQuery( + const { data: instances, dataUpdatedAt } = usePrefetchedApiQuery( 'instanceList', { query: { project, limit: PAGE_SIZE } }, { - // The point of all this is to poll for a certain amount of time after - // some instance in the current page is transitioning. For example, if - // you manually stop an instance, its state will change to `stopping`, - // which will cause this logic to start polling the list until it lands - // in `stopped`, which is not a transitional state. We want polling to - // eventually time out so we're not polling forever if an instance is - // stuck in starting or whatever. + // The point of all this is to poll quickly for a certain amount of time + // after some instance in the current page enters a transitional state + // like starting or stopping. After that, it will keep polling, but more + // slowly. For example, if you stop an instance, its state will change to + // `stopping`, which will cause this logic to start polling the list until + // it lands in `stopped`, at which point it will stop polling because + // `stopped` is not considered transitional. refetchInterval({ state: { data } }) { const prevTransitioning = transitioningInstances.current const nextTransitioning = new Set( @@ -105,28 +108,20 @@ export function InstancesPage() { .map((i) => i.id + '|' + i.runState) ) - // always update. we don't have to worry about doing this in all the branches below. + // always update the ledger to the current state transitioningInstances.current = nextTransitioning - // We use this setDiff logic instead of just checking whether the set - // has changed because if you have two transitioning instances and - // one stops transitioning, then that's a change in the set, but you - // shouldn't start polling because of it! What you want to look for is - // new transitioning instances. - const anyTransitioning = nextTransitioning.size > 0 + // We use this set difference logic instead of set equality because if + // you have two transitioning instances and one stops transitioning, + // then that's a change in the set, but you shouldn't start polling + // fast because of it! What you want to look for is *new* transitioning + // instances. const anyNewTransitioning = setDiff(nextTransitioning, prevTransitioning).size > 0 + if (anyNewTransitioning) pollingStartTime.current = Date.now() - if (anyTransitioning) { - // if we have new transitioning instances, restart the timeout clock - if (anyNewTransitioning) pollingStartTime.current = Date.now() - - // important that elapsed is calculated *after* bumping start time - const elapsed = Date.now() - pollingStartTime.current - if (elapsed < POLL_TIMEOUT) return POLL_INTERVAL - // otherwise fall through to false below - } - - return false + // important that elapsed is calculated *after* potentially bumping start time + const elapsed = Date.now() - pollingStartTime.current + return elapsed < POLL_FAST_TIMEOUT ? POLL_INTERVAL_FAST : POLL_INTERVAL_SLOW }, } ) @@ -205,8 +200,20 @@ export function InstancesPage() { links={[docLinks.instances, docLinks.instanceActions]} /> - - + {/* Avoid changing justify-end on TableActions for this one case. We can + * fix this properly when we add refresh and filtering for all tables. */} + +
+ + + + Updated {toLocaleTimeString(new Date(dataUpdatedAt))} + + +
New Instance
} /> diff --git a/app/pages/project/instances/instance/InstancePage.tsx b/app/pages/project/instances/instance/InstancePage.tsx index 95c2b2d196..5b0a1ef8de 100644 --- a/app/pages/project/instances/instance/InstancePage.tsx +++ b/app/pages/project/instances/instance/InstancePage.tsx @@ -172,7 +172,7 @@ export function InstancePage() {
{polling && ( - + diff --git a/app/ui/lib/Table.tsx b/app/ui/lib/Table.tsx index 07f71797d3..53786f75df 100644 --- a/app/ui/lib/Table.tsx +++ b/app/ui/lib/Table.tsx @@ -115,7 +115,7 @@ Table.Cell = ({ height = 'small', className, children, ...props }: TableCellProp * Used _outside_ of the `Table`, this element wraps buttons that sit on top * of the table. */ -export const TableActions = classed.div`-mt-11 mb-3 flex justify-end space-x-2` +export const TableActions = classed.div`-mt-11 mb-3 flex justify-end gap-2` export const TableEmptyBox = classed.div`flex h-full max-h-[480px] items-center justify-center rounded-lg border border-secondary p-4` From 4fe0d0519793d35e4b6582e72bc06857cd2e9abf Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 23 Aug 2024 10:55:26 -0500 Subject: [PATCH 07/11] test polling in e2es --- app/pages/project/instances/InstancesPage.tsx | 1 + .../instances/instance/InstancePage.tsx | 4 +- test/e2e/instance.e2e.ts | 59 +++++++++++++------ test/e2e/utils.ts | 7 +-- 4 files changed, 45 insertions(+), 26 deletions(-) diff --git a/app/pages/project/instances/InstancesPage.tsx b/app/pages/project/instances/InstancesPage.tsx index 6c8a9773c1..a1bfcc8342 100644 --- a/app/pages/project/instances/InstancesPage.tsx +++ b/app/pages/project/instances/InstancesPage.tsx @@ -61,6 +61,7 @@ InstancesPage.loader = async ({ params }: LoaderFunctionArgs) => { const sec = 1000 // ms, obviously const POLL_FAST_TIMEOUT = 30 * sec +// a little slower than instance detail because this is a bigger response const POLL_INTERVAL_FAST = 3 * sec const POLL_INTERVAL_SLOW = 60 * sec diff --git a/app/pages/project/instances/instance/InstancePage.tsx b/app/pages/project/instances/instance/InstancePage.tsx index 5b0a1ef8de..871a6f82f2 100644 --- a/app/pages/project/instances/instance/InstancePage.tsx +++ b/app/pages/project/instances/instance/InstancePage.tsx @@ -87,6 +87,8 @@ InstancePage.loader = async ({ params }: LoaderFunctionArgs) => { return null } +const POLL_INTERVAL = 1000 + export function InstancePage() { const instanceSelector = useInstanceSelector() @@ -105,7 +107,7 @@ export function InstancePage() { }, { refetchInterval: ({ state: { data: instance } }) => - instance && instanceTransitioning(instance) ? 1000 : false, + instance && instanceTransitioning(instance) ? POLL_INTERVAL : false, } ) diff --git a/test/e2e/instance.e2e.ts b/test/e2e/instance.e2e.ts index 204bd22bd9..65dbac0a76 100644 --- a/test/e2e/instance.e2e.ts +++ b/test/e2e/instance.e2e.ts @@ -5,7 +5,7 @@ * * Copyright Oxide Computer Company */ -import { expect, expectRowVisible, refreshInstance, sleep, test } from './utils' +import { clickRowAction, expect, expectRowVisible, test } from './utils' test('can delete a failed instance', async ({ page }) => { await page.goto('/projects/mock-project/instances') @@ -26,9 +26,12 @@ test('can delete a failed instance', async ({ page }) => { test('can stop and delete a running instance', async ({ page }) => { await page.goto('/projects/mock-project/instances') + const table = page.getByRole('table') + await expectRowVisible(table, { + name: 'db1', + status: expect.stringContaining('running'), + }) const row = page.getByRole('row', { name: 'db1', exact: false }) - await expect(row).toBeVisible() - await expect(row.getByRole('cell', { name: /running/ })).toBeVisible() // can't delete, can stop await row.getByRole('button', { name: 'Row actions' }).click() @@ -36,35 +39,53 @@ test('can stop and delete a running instance', async ({ page }) => { await page.getByRole('menuitem', { name: 'Stop' }).click() await page.getByRole('button', { name: 'Confirm' }).click() - await sleep(4000) - await refreshInstance(page) - - // now it's stopped - await expect(row.getByRole('cell', { name: /stopped/ })).toBeVisible() + // polling makes it go stopping and then stopped + await expectRowVisible(table, { + name: 'db1', + status: expect.stringContaining('stopping'), + }) + await expectRowVisible(table, { + name: 'db1', + status: expect.stringContaining('stopped'), + }) // now delete - await row.getByRole('button', { name: 'Row actions' }).click() - await page.getByRole('menuitem', { name: 'Delete' }).click() + await clickRowAction(page, 'db1', 'Delete') await page.getByRole('button', { name: 'Confirm' }).click() await expect(row).toBeHidden() // bye }) -test('can stop a starting instance', async ({ page }) => { +test('can stop a starting instance, then start it again', async ({ page }) => { await page.goto('/projects/mock-project/instances') - const row = page.getByRole('row', { name: 'not-there-yet', exact: false }) - await expect(row).toBeVisible() - await expect(row.getByRole('cell', { name: /starting/ })).toBeVisible() + const table = page.getByRole('table') + await expectRowVisible(table, { + name: 'not-there-yet', + status: expect.stringContaining('starting'), + }) - await row.getByRole('button', { name: 'Row actions' }).click() - await page.getByRole('menuitem', { name: 'Stop' }).click() + await clickRowAction(page, 'not-there-yet', 'Stop') await page.getByRole('button', { name: 'Confirm' }).click() - await sleep(4000) - await refreshInstance(page) + await expectRowVisible(table, { + name: 'not-there-yet', + status: expect.stringContaining('stopping'), + }) + await expectRowVisible(table, { + name: 'not-there-yet', + status: expect.stringContaining('stopped'), + }) - await expect(row.getByRole('cell', { name: /stopped/ })).toBeVisible() + await clickRowAction(page, 'not-there-yet', 'Stop') + await expectRowVisible(table, { + name: 'not-there-yet', + status: expect.stringContaining('starting'), + }) + await expectRowVisible(table, { + name: 'not-there-yet', + status: expect.stringContaining('running'), + }) }) test('delete from instance detail', async ({ page }) => { diff --git a/test/e2e/utils.ts b/test/e2e/utils.ts index 998a561937..5d4da1dfc0 100644 --- a/test/e2e/utils.ts +++ b/test/e2e/utils.ts @@ -111,15 +111,10 @@ export async function stopInstance(page: Page) { await page.getByRole('menuitem', { name: 'Stop' }).click() await page.getByRole('button', { name: 'Confirm' }).click() await closeToast(page) - await sleep(2000) - await refreshInstance(page) + // don't need to manually refresh because of polling await expect(page.getByText('statusstopped')).toBeVisible() } -export async function refreshInstance(page: Page) { - await page.getByRole('button', { name: 'Refresh data' }).click() -} - /** * Close toast and wait for it to fade out. For some reason it prevents things * from working, but only in tests as far as we can tell. From 91d8ba29f756146f8709e3ee2b2e945b30e7912c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 23 Aug 2024 11:02:58 -0500 Subject: [PATCH 08/11] forgot that we only want to bother polling fast if there are any instances transitioning --- app/pages/project/instances/InstancesPage.tsx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/pages/project/instances/InstancesPage.tsx b/app/pages/project/instances/InstancesPage.tsx index a1bfcc8342..149716a6aa 100644 --- a/app/pages/project/instances/InstancesPage.tsx +++ b/app/pages/project/instances/InstancesPage.tsx @@ -117,12 +117,17 @@ export function InstancesPage() { // then that's a change in the set, but you shouldn't start polling // fast because of it! What you want to look for is *new* transitioning // instances. + const anyTransitioning = nextTransitioning.size > 0 const anyNewTransitioning = setDiff(nextTransitioning, prevTransitioning).size > 0 + + // if there are new instances in transitioning, restart the timeout window if (anyNewTransitioning) pollingStartTime.current = Date.now() // important that elapsed is calculated *after* potentially bumping start time const elapsed = Date.now() - pollingStartTime.current - return elapsed < POLL_FAST_TIMEOUT ? POLL_INTERVAL_FAST : POLL_INTERVAL_SLOW + return anyTransitioning && elapsed < POLL_FAST_TIMEOUT + ? POLL_INTERVAL_FAST + : POLL_INTERVAL_SLOW }, } ) From 190d17eb28278d01839b786d4fdcf31ca82c39d9 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 23 Aug 2024 11:09:07 -0500 Subject: [PATCH 09/11] comment tweak --- app/pages/project/instances/InstancesPage.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/pages/project/instances/InstancesPage.tsx b/app/pages/project/instances/InstancesPage.tsx index 149716a6aa..920958ac13 100644 --- a/app/pages/project/instances/InstancesPage.tsx +++ b/app/pages/project/instances/InstancesPage.tsx @@ -93,7 +93,7 @@ export function InstancesPage() { // like starting or stopping. After that, it will keep polling, but more // slowly. For example, if you stop an instance, its state will change to // `stopping`, which will cause this logic to start polling the list until - // it lands in `stopped`, at which point it will stop polling because + // it lands in `stopped`, at which point it will poll only slowly because // `stopped` is not considered transitional. refetchInterval({ state: { data } }) { const prevTransitioning = transitioningInstances.current From 1a9067be48bf79e7a48a1bec0dce685be2f8dc22 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 23 Aug 2024 11:35:43 -0500 Subject: [PATCH 10/11] fix typo in test. guitly --- test/e2e/instance.e2e.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/instance.e2e.ts b/test/e2e/instance.e2e.ts index 65dbac0a76..5bb7be2bb1 100644 --- a/test/e2e/instance.e2e.ts +++ b/test/e2e/instance.e2e.ts @@ -77,7 +77,7 @@ test('can stop a starting instance, then start it again', async ({ page }) => { status: expect.stringContaining('stopped'), }) - await clickRowAction(page, 'not-there-yet', 'Stop') + await clickRowAction(page, 'not-there-yet', 'Start') await expectRowVisible(table, { name: 'not-there-yet', status: expect.stringContaining('starting'), From 667eb5a7bd29ecadc521ac91dab80bbe67e73d32 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 23 Aug 2024 11:42:28 -0500 Subject: [PATCH 11/11] stray test.only --- app/util/array.spec.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/util/array.spec.tsx b/app/util/array.spec.tsx index f1b12bacf4..1a2f4bf295 100644 --- a/app/util/array.spec.tsx +++ b/app/util/array.spec.tsx @@ -73,7 +73,7 @@ test('isSetEqual', () => { expect(isSetEqual(new Set([{}]), new Set([{}]))).toBe(false) }) -test.only('setDiff', () => { +test('setDiff', () => { expect(setDiff(new Set(), new Set())).toEqual(new Set()) expect(setDiff(new Set(['a']), new Set())).toEqual(new Set(['a'])) expect(setDiff(new Set(), new Set(['a']))).toEqual(new Set())