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.

Sign up for GitHub

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

Jump to bottom

test: migrate Editor tests to RTL #1624

Merged
merged 2 commits into from
Aug 20, 2024
Merged

test: migrate Editor tests to RTL #1624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1437e6b
test: move `Editor` test to `rtl-spec`
erikian Aug 3, 2024
6b06cdf
test: migrate `Editor` tests to RTL
erikian Aug 3, 2024
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
89 changes: 47 additions & 42 deletions tests/renderer/components/editor-spec.tsx → rtl-spec/components/editor.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import * as React from 'react';

import { shallow } from 'enzyme';

import { EditorId, MAIN_JS } from '../../../src/interfaces';
import { Editor } from '../../../src/renderer/components/editor';
import { AppState } from '../../../src/renderer/state';
import { EditorId, MAIN_JS } from '../../src/interfaces';
import { Editor } from '../../src/renderer/components/editor';
import { AppState } from '../../src/renderer/state';
import { renderClassComponentWithInstanceRef } from '../test-utils/renderClassComponentWithInstanceRef';

type DidMount = () => void;

Expand All @@ -18,23 +15,27 @@ describe('Editor component', () => {
});

function createEditor(id: EditorId, didMount: DidMount = jest.fn()) {
const wrapper = shallow(
<Editor
appState={store}
editorDidMount={didMount}
id={id}
monaco={monaco}
monacoOptions={{}}
setFocused={() => undefined}
/>,
);
const instance: any = wrapper.instance();
return { wrapper, instance };
return renderClassComponentWithInstanceRef(Editor, {
appState: store,
editorDidMount: didMount,
id,
monaco,
monacoOptions: {},
setFocused: () => undefined,
});
}

function initializeEditorMosaic(id: EditorId) {
store.editorMosaic.set({ [id]: '// content' });
}

it('renders the editor container', () => {
const { wrapper } = createEditor(MAIN_JS);
expect(wrapper.html()).toBe('<div class="editorContainer"></div>');
const id = MAIN_JS;
initializeEditorMosaic(id);

const { renderResult } = createEditor(id);

expect(renderResult.getByTestId('editorContainer')).toBeInTheDocument();
});
Comment on lines 32 to 39
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this test is serving much purpose since it's checking for an empty div. Personally I'd rather drop it and avoid the need for testid here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we're removing snapshot-based testing, I think these renders the component tests could at least act as basic smoke tests — for instance, if we change a component to render conditionally and it suddenly doesn't render anything with the props we're providing in the test, or if the component now crashes before rendering given the same props, the renders the component test will fail and we'll know right away that there's something wrong when mounting the component; that wouldn't be immediately obvious for a failure caused by bad props / bad prop validation in, say, the correctly sets the language test.

I don't feel strongly about this though, so I'm happy to remove this test if we think it's fine to have other tests fail in case the component fails to render — I was mostly trying to keep the tests we already had in some way.

Copy link
Member

Choose a reason for hiding this comment

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

Now that we're removing snapshot-based testing, I think these renders the component tests could at least act as basic smoke tests

I agree. This one is just an extreme case where it's just an empty div. I'd like to avoid needing to add data-testid, but I'm fine with keeping it in this case, I don't feel strongly on this either.


describe('correctly sets the language', () => {
Expand All @@ -43,13 +44,20 @@ describe('Editor component', () => {
['for html', 'file.html', 'html'],
['for css', 'file.css', 'css'],
])('%s', (_: unknown, filename: EditorId, language: string) => {
initializeEditorMosaic(filename);

const { instance } = createEditor(filename);

expect(instance.language).toBe(language);
});
});

it('denies updates', () => {
const { instance } = createEditor(MAIN_JS);
const id = MAIN_JS;
initializeEditorMosaic(id);

const { instance } = createEditor(id);

expect(instance.shouldComponentUpdate()).toBe(false);
});

Expand All @@ -61,48 +69,45 @@ describe('Editor component', () => {
const addEditorSpy = jest.spyOn(editorMosaic, 'addEditor');

const didMount = jest.fn();
const { instance } = createEditor(id, didMount);

instance.containerRef.current = 'ref';
await instance.initMonaco();
createEditor(id, didMount);

expect(didMount).toHaveBeenCalled();
expect(addEditorSpy).toHaveBeenCalledWith(id, expect.anything());
});

it('sets up a listener on focused text editor', async () => {
const id = MAIN_JS;
store.editorMosaic.set({ [id]: '// content' });
const { instance } = createEditor(id);
initializeEditorMosaic(id);
createEditor(id);

instance.containerRef.current = 'ref';
await instance.initMonaco();
expect(monaco.latestEditor.onDidFocusEditorText).toHaveBeenCalled();
});
});

it('componentWillUnmount() attempts to dispose the editor', async () => {
const id = MAIN_JS;
store.editorMosaic.set({ [id]: '// content' });
const didMount = jest.fn();
const { instance } = createEditor(id, didMount);
initializeEditorMosaic(id);

const {
renderResult: { unmount },
} = createEditor(id);

instance.containerRef.current = 'ref';
await instance.initMonaco();
instance.componentWillUnmount();
unmount();

expect(monaco.latestEditor.dispose).toHaveBeenCalled();
});

it('focus editor file', async () => {
const id = MAIN_JS;
store.editorMosaic.set({ [id]: '// content' });
const didMount = jest.fn();
const { instance } = createEditor(id, didMount);
initializeEditorMosaic(id);

const {
instance,
renderResult: { unmount },
} = createEditor(id);

unmount();

instance.containerRef.current = 'ref';
await instance.initMonaco();
instance.componentWillUnmount();
expect(instance.props.id).toBe(id);
});
});
8 changes: 7 additions & 1 deletion src/renderer/components/editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,12 @@ export class Editor extends React.Component<EditorProps> {
}

public render() {
return <div className="editorContainer" ref={this.containerRef} />;
return (
<div
className="editorContainer"
data-testid="editorContainer"
ref={this.containerRef}
/>
);
}
}