-
Notifications
You must be signed in to change notification settings - Fork 716
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
test: migrate Editors
tests to RTL
#1625
base: main
Are you sure you want to change the base?
Conversation
5a02bbf
to
9449eec
Compare
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.
As much as possible we should avoid relying on testid
, to best align with the guiding principles of Testing Library. I think with a bit of refactoring we can avoid it in this case (which would remove the need for TestIdContainer
in this PR).
diff --git a/rtl-spec/components/editors.spec.tsx b/rtl-spec/components/editors.spec.tsx
index 887c3433..1928e721 100644
--- a/rtl-spec/components/editors.spec.tsx
+++ b/rtl-spec/components/editors.spec.tsx
@@ -5,6 +5,7 @@ import { App } from '../../src/renderer/app';
import { Editors } from '../../src/renderer/components/editors';
import { Editor, EditorMosaic } from '../../src/renderer/editor-mosaic';
import { AppState } from '../../src/renderer/state';
+import { getEditorTitle } from '../../src/renderer/utils/editor-utils';
import {
MonacoEditorMock,
StateMock,
@@ -14,7 +15,7 @@ import { emitEvent } from '../../tests/utils';
import { renderClassComponentWithInstanceRef } from '../test-utils/renderClassComponentWithInstanceRef';
jest.mock('../../src/renderer/components/editor', () => ({
- Editor: () => 'Editor',
+ Editor: () => 'EditorComponent',
}));
describe('Editors component', () => {
@@ -42,7 +43,7 @@ describe('Editors component', () => {
it('renders', () => {
const { renderResult } = renderEditors();
- expect(renderResult.getByTestId('editors')).toBeInTheDocument();
+ expect(renderResult.getAllByText('EditorComponent')).not.toHaveLength(0);
});
it('does not execute command if not supported', () => {
@@ -91,19 +92,16 @@ describe('Editors component', () => {
it('renders toolbars', () => {
const { renderResult } = renderEditors();
- const [
- mainToolbar,
- rendererToolbar,
- htmlToolbar,
- preloadToolbar,
- stylesheetToolbar,
- ] = renderResult.getAllByTestId('editors-toolbar');
-
- expect(mainToolbar).toHaveTextContent('Main Process (main.js)');
- expect(rendererToolbar).toHaveTextContent('Renderer Process (renderer.js)');
- expect(htmlToolbar).toHaveTextContent('HTML (index.html)');
- expect(preloadToolbar).toHaveTextContent('Preload (preload.js)');
- expect(stylesheetToolbar).toHaveTextContent('Stylesheet (styles.css)');
+ const toolbars = renderResult.getAllByRole('toolbar');
+ const editors = Object.keys(editorValues);
+ expect(editors).not.toHaveLength(0);
+
+ for (const editorId of editors) {
+ const title = getEditorTitle(editorId as EditorId);
+ expect(
+ toolbars.find((toolbar) => toolbar.textContent?.includes(title)),
+ ).toBeInTheDocument();
+ }
});
it('onChange() updates the mosaic arrangement in the appState', () => {
diff --git a/src/renderer/components/editors.tsx b/src/renderer/components/editors.tsx
index c1cb0a7a..16050ea4 100644
--- a/src/renderer/components/editors.tsx
+++ b/src/renderer/components/editors.tsx
@@ -192,7 +192,7 @@ export const Editors = observer(
const { appState } = this.props;
return (
- <div data-testid="editors-toolbar">
+ <div role="toolbar">
{/* Left */}
<div>
<h5>{title}</h5>
There's an additional change in there to avoid relying on the order of the editors when checking for the toolbars.
Ref: #1408.