Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add empty and loading states to Plots webview with tests #1000

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions extension/src/webview/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export const createWebview = async (
title,
ViewColumn.Active,
{
enableCommandUris: true,
enableScripts: true,
localResourceRoots: [Uri.file(distPath)],
retainContextWhenHidden: true
Expand Down
78 changes: 78 additions & 0 deletions webview/src/plots/components/App.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/**
* @jest-environment jsdom
*/
import React from 'react'
import {
render,
cleanup,
waitFor,
screen,
fireEvent
} from '@testing-library/react'
import '@testing-library/jest-dom/extend-expect'
import complexLivePlotsData from 'dvc/src/test/fixtures/complex-live-plots-example'
import {
MessageFromWebviewType,
MessageToWebviewType
} from 'dvc/src/webview/contract'
import { App } from './App'

import { vsCodeApi } from '../util/vscode'

jest.mock('../util/vscode')

const { postMessage: mockPostMessage } = vsCodeApi
beforeEach(() => {
jest.clearAllMocks()
})

afterEach(() => {
cleanup()
})

describe('App', () => {
it('Sends the initialized message on first render', () => {
render(<App />)
expect(mockPostMessage).toHaveBeenCalledWith({
type: MessageFromWebviewType.initialized
})
expect(mockPostMessage).toHaveBeenCalledTimes(1)
})

it('Renders the loading state when given no data', async () => {
render(<App />)
const loadingState = await waitFor(() =>
screen.getByText('Loading plots data...')
)
expect(loadingState).toBeInTheDocument()
})

it('Renders the empty state when given data with no experiments', async () => {
const dataMessageWithoutPlots = new MessageEvent('message', {
data: {
data: [],
type: MessageToWebviewType.setData
}
})
render(<App />)
fireEvent(window, dataMessageWithoutPlots)
const emptyState = await waitFor(() =>
screen.getByText('There are no experiments to make plots from.')
)
expect(emptyState).toBeInTheDocument()
})

it('Renders plots when given a message with plots data', () => {
const dataMessageWithPlots = new MessageEvent('message', {
data: {
data: complexLivePlotsData,
type: MessageToWebviewType.setData
}
})
render(<App />)
fireEvent(window, dataMessageWithPlots)

const emptyState = screen.queryByText('Loading plots data...')
expect(emptyState).not.toBeInTheDocument()
})
})
27 changes: 13 additions & 14 deletions webview/src/plots/components/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,24 @@ import React, { useEffect, useState } from 'react'
import { PlotsData } from 'dvc/src/plots/webview/contract'
import {
MessageFromWebviewType,
MessageToWebview,
MessageToWebviewType
} from 'dvc/src/webview/contract'
import Plots from './Plots'
import { InternalVsCodeApi } from '../../shared/api'

declare global {
function acquireVsCodeApi(): InternalVsCodeApi
}

const vsCodeApi = acquireVsCodeApi()
import { vsCodeApi } from '../util/vscode'

const signalInitialized = () =>
vsCodeApi.postMessage({ type: MessageFromWebviewType.initialized })

const App = () => {
export const App = () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function App has 29 lines of code (exceeds 25 allowed). Consider refactoring.

const [plotsData, setPlotsData] = useState<PlotsData>()
const [dvcRoot, setDvcRoot] = useState()
const [dvcRoot, setDvcRoot] = useState<string>()
useEffect(() => {
signalInitialized()
window.addEventListener('message', ({ data }) => {
const messageListener = ({
data
}: {
data: MessageToWebview<PlotsData>
}) => {
switch (data.type) {
case MessageToWebviewType.setData: {
setPlotsData(data.data)
Expand All @@ -31,7 +29,10 @@ const App = () => {
setDvcRoot(data.dvcRoot)
}
}
})
}
window.addEventListener('message', messageListener)
signalInitialized()
return () => window.removeEventListener('message', messageListener)
Comment on lines +32 to +35
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaning up a memory leak that was exposed by tests re-creating the App in the same context.

}, [])
useEffect(() => {
vsCodeApi.setState({
Expand All @@ -41,5 +42,3 @@ const App = () => {
}, [plotsData, dvcRoot])
return <Plots plotsData={plotsData} />
}

export default App
12 changes: 11 additions & 1 deletion webview/src/plots/components/Plots.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,10 @@ const Plot = ({
}

const Plots = ({ plotsData }: { plotsData?: PlotsData }) => {
return (
if (!plotsData) {
return <p>Loading plots data...</p>
}
return plotsData.length > 0 ? (
<>
{plotsData?.map(plotData => (
<Plot
Expand All @@ -121,6 +124,13 @@ const Plots = ({ plotsData }: { plotsData?: PlotsData }) => {
/>
))}
</>
) : (
<>
<p>There are no experiments to make plots from.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Could we wrap this in a centered div and make it h-something to make it look more VS Code-ish?

Like:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I did the same thing we do for the table with no data, but the ugliness is more apparent here since this state can happen much more often.

<p>
<a href="command:dvc.runExperiment">Run an Experiment</a> to add some!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I] This is not necessarily true (no checkpoints).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also need to pass dvcRoot (multi-repo workspace)

</p>
</>
)
}
export default Plots
2 changes: 1 addition & 1 deletion webview/src/plots/index.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react'
import ReactDOM from 'react-dom'
import App from './components/App'
import { App } from './components/App'

const elem = document.createElement('div')
elem.className = 'react-root'
Expand Down
5 changes: 5 additions & 0 deletions webview/src/plots/util/__mocks__/vscode.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const vsCodeApi = {
getState: jest.fn(),
postMessage: jest.fn(),
setState: jest.fn()
}
7 changes: 7 additions & 0 deletions webview/src/plots/util/vscode.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { InternalVsCodeApi } from '../../shared/api'

declare global {
function acquireVsCodeApi(): InternalVsCodeApi
}

export const vsCodeApi = acquireVsCodeApi()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't find a way to mock this global directly, so wrapping it is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Did you see that we mock this for experiments? See webview/src/experiments/components/App.test.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also mock a wrapper there, webview/src/experiments/model/vsCodeApi.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not reuse the code?

4 changes: 4 additions & 0 deletions webview/src/stories/Plots.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,7 @@ export const WithData: Story<{ plotsData: PlotsData }> = ({ plotsData }) => {
export const WithoutData: Story = () => {
return <Plots plotsData={undefined} />
}

export const WithoutExperiments: Story = () => {
return <Plots plotsData={[]} />
}