[Lens] Data panel styling and optimizations #40787
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| import React from 'react'; | ||
| import { mountWithIntl as mount } from 'test_utils/enzyme_helpers'; | ||
| import { debouncedComponent } from './debounced_component'; | ||
|
|
||
| describe('debouncedComponent', () => { | ||
| test('immediately renders', () => { | ||
| const TestComponent = debouncedComponent(({ title }: { title: string }) => { | ||
| return <h1>{title}</h1>; | ||
| }); | ||
| expect(mount(<TestComponent title="hoi" />).html()).toMatchInlineSnapshot(`"<h1>hoi</h1>"`); | ||
| }); | ||
|
|
||
| test('debounces changes', async () => { | ||
| const TestComponent = debouncedComponent(({ title }: { title: string }) => { | ||
| return <h1>{title}</h1>; | ||
| }, 1); | ||
| const component = mount(<TestComponent title="there" />); | ||
| component.setProps({ title: 'yall' }); | ||
| expect(component.html()).toMatchInlineSnapshot(`"<h1>there</h1>"`); | ||
| await new Promise(r => setTimeout(r, 1)); | ||
| expect(component.html()).toMatchInlineSnapshot(`"<h1>yall</h1>"`); | ||
| }); | ||
| }); | ||
|
There was a problem hiding this comment. These snapshot tests are really weak- please assert something specific about the content, such as: There was a problem hiding this comment. I find the snapshot to be more effective than the examples you gave. Maybe we can discuss. There was a problem hiding this comment. The reason I say they're weak is that snapshots aren't testing the behavior of the system. If I change the behavior here, I want tests to fail. Instead of tests failing with a specific message, it says that the snapshots have changed and I see a diff that I have to interpret myself. There's no assertion of specific behavior. For example, the issue with the SeriesType in the XY chart was made worse by snapshot testing. We were snapshotting the rendered chart, but without any assertions that the right series was rendered. So until I fixed it, we were rendering the My preferred solution is combination, like I did for the SeriesType issue: test('it renders stacked horizontal bar', () => {
const { data, args } = sampleArgs();
const component = shallow(
<XYChart data={data} args={{ ...args, seriesType: 'horizontal_bar_stacked' }} />
);
expect(component).toMatchSnapshot();
expect(component.find(BarSeries)).toHaveLength(1);
expect(component.find(BarSeries).prop('stackAccessors')).toHaveLength(1);
expect(component.find(Settings).prop('rotation')).toEqual(90);
});That kind of test will catch both unexpected additions and fail fast if the required behavior changes. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| import React, { useState, useMemo, memo, FunctionComponent } from 'react'; | ||
| import { debounce } from 'lodash'; | ||
|
|
||
| export function debouncedComponent<TProps>(component: FunctionComponent<TProps>, delay = 256) { | ||
| const MemoizedComponent = (memo(component) as unknown) as FunctionComponent<TProps>; | ||
|
|
||
| return memo((props: TProps) => { | ||
| const [rendered, setRendered] = useState(props); | ||
| const delayRender = useMemo(() => debounce(setRendered, delay), []); | ||
|
|
||
| delayRender(props); | ||
|
|
||
| return React.createElement(MemoizedComponent, rendered); | ||
|
There was a problem hiding this comment. I wasn't reading this correctly the first time around, it seems like There was a problem hiding this comment. Yeah. Good point. Rendered is a terrible name. |
||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| export * from './debounced_component'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm skeptical of any tests using real timers, even if this is effectively immediate. I would recommend using
jest.useFakeTimers(): https://jestjs.io/docs/en/timer-mocksThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When possible, I prefer to use the real thing, but I'm not strongly opinionated on this.
Edit: Doesn't seem to work, due to the internals of debounce.