Skip to content

Commit

Permalink
Fix teleporter crash and integration tests (#11656)
Browse files Browse the repository at this point in the history
Fix failing integration tests:
- Fix a Vue Teleporter crash that became reachable when the dropdown arrow is displayed more often (#11620).
- Fix a new drag-and-drop test that didn't work in CI.
- Update mock data for multi-type expression updates (#11583).

# Important Notes
- The new `ConditionalTeleport` component should be used for any `Teleport` that uses the `disabled` prop and has a `to` that isn't always a valid teleportation target.
  • Loading branch information
kazcw authored Nov 26, 2024
1 parent 582a9aa commit 1900299
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 12 deletions.
2 changes: 1 addition & 1 deletion app/gui/e2e/project-view/tableVisualisation.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { graphNodeByBinding } from './locate'
/** Prepare the graph for the tests. We add the table type to the `aggregated` node. */
async function initGraph(page: Page) {
await actions.goToGraph(page)
await mockExpressionUpdate(page, 'aggregated', { type: 'Standard.Table.Table.Table' })
await mockExpressionUpdate(page, 'aggregated', { type: ['Standard.Table.Table.Table'] })
}

/**
Expand Down
8 changes: 4 additions & 4 deletions app/gui/e2e/project-view/typesOnNodeHover.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ test('shows the correct type when hovering a node', async ({ page }) => {
await actions.goToGraph(page)

// Note that the types don't have to make sense, they just have to be applied.
await mockExpressionUpdate(page, 'five', { type: DUMMY_INT_TYPE.full })
await mockExpressionUpdate(page, 'ten', { type: DUMMY_STRING_TYPE.full })
await mockExpressionUpdate(page, 'sum', { type: DUMMY_FLOAT_TYPE.full })
await mockExpressionUpdate(page, 'prod', { type: DUMMY_INT_TYPE.full })
await mockExpressionUpdate(page, 'five', { type: [DUMMY_INT_TYPE.full] })
await mockExpressionUpdate(page, 'ten', { type: [DUMMY_STRING_TYPE.full] })
await mockExpressionUpdate(page, 'sum', { type: [DUMMY_FLOAT_TYPE.full] })
await mockExpressionUpdate(page, 'prod', { type: [DUMMY_INT_TYPE.full] })

await assertTypeLabelOnNodeByBinding(page, 'five', DUMMY_INT_TYPE)
await assertTypeLabelOnNodeByBinding(page, 'ten', DUMMY_STRING_TYPE)
Expand Down
15 changes: 10 additions & 5 deletions app/gui/e2e/project-view/widgets.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,12 +216,17 @@ test('Editing list', async ({ page }) => {
// Test drag: reorder items
await vectorItems.nth(1).locator('[draggable]').hover()
await page.mouse.down()
await vectorItems
.nth(1)
.locator('[draggable]')
.hover({ position: { x: 10, y: 10 } })
// `dragenter` / `dragleave` events are not dispatched reliably without multiple mouse movements
await vectorElements.first().hover({ position: { x: 10, y: 10 }, force: true })
await vectorElements.first().hover({ position: { x: 20, y: 10 }, force: true })
await vectorElements.first().hover({ position: { x: 30, y: 10 }, force: true })
await locate.graphEditor(page).hover({ position: { x: 100, y: 300 } })
await expect(vectorElements).toHaveText(['..Group_By'])
await expect(vector.getByTestId('dragPlaceholder')).toHaveCount(0)
await vectorElements.first().hover({ position: { x: 10, y: 10 }, force: true })
await vectorElements.first().hover({ position: { x: 20, y: 10 }, force: true })
await vectorElements.first().hover({ position: { x: 30, y: 10 }, force: true })
await expect(vector.getByTestId('dragPlaceholder')).toHaveCount(1)
await page.mouse.up()
await expect(vectorElements).toHaveText(['_', '..Group_By'])

Expand Down Expand Up @@ -579,7 +584,7 @@ test('Autoscoped constructors', async ({ page }) => {
await node.click()
await expect(topLevelArgs).toHaveCount(3)

const groupBy = node.getByTestId('list-item-content').first()
const groupBy = node.getByTestId('list-item-content')
await expect(groupBy).toBeVisible()
await expect(groupBy.locator('.WidgetArgumentName')).toContainText(['column', 'new_name'])
})
Expand Down
20 changes: 20 additions & 0 deletions app/gui/src/project-view/components/ConditionalTeleport.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<script setup lang="ts">
/**
* @file This component is equivalent to `Teleport` except when `disabled` is `true`, `to` doesn't have to specify a
* valid teleportation target (works around a Vue issue present at least in 3.5.2-3.5.13).
*/
import { type RendererElement } from 'vue'
const { disabled } = defineProps<{
disabled: boolean
to: string | RendererElement | null | undefined
}>()
</script>

<template>
<!-- Note: `defer` must not be used here, or Vue errors will occur when unmounting components as of 3.5.2. -->
<Teleport v-if="!disabled" :to="to">
<slot />
</Teleport>
<slot v-else />
</template>
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<script setup lang="ts">
import ConditionalTeleport from '@/components/ConditionalTeleport.vue'
import NodeWidget from '@/components/GraphEditor/NodeWidget.vue'
import { enclosingTopLevelArgument } from '@/components/GraphEditor/widgets/WidgetTopLevelArgument.vue'
import SizeTransition from '@/components/SizeTransition.vue'
Expand Down Expand Up @@ -463,13 +464,13 @@ declare module '@/providers/widgetRegistry' {
@pointerout="isHovered = false"
>
<NodeWidget :input="innerWidgetInput" />
<Teleport v-if="showArrow" defer :disabled="!arrowLocation" :to="arrowLocation">
<ConditionalTeleport v-if="showArrow" :disabled="!arrowLocation" :to="arrowLocation">
<SvgIcon
name="arrow_right_head_only"
class="arrow widgetOutOfLayout"
:class="{ hovered: isHovered }"
/>
</Teleport>
</ConditionalTeleport>
<Teleport v-if="tree.nodeElement" :to="tree.nodeElement">
<div ref="dropdownElement" :style="floatingStyles" class="widgetOutOfLayout floatingElement">
<SizeTransition height :duration="100">
Expand Down
1 change: 1 addition & 0 deletions app/gui/src/project-view/components/widgets/ListWidget.vue
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,7 @@ function deleteItem(index: number) {
<template v-else>
<li
:ref="patchBoundingClientRectScaling"
data-testid="dragPlaceholder"
class="placeholder"
:style="{ '--placeholder-width': entry.width + 'px' }"
></li>
Expand Down

0 comments on commit 1900299

Please sign in to comment.