test: migrate Editor tests to RTL
#1624
Conversation
erikian
force-pushed
the
test/rtl-editor-component
branch
from
f3baf06
to
6b06cdf
Compare
| 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(); | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Ref: #1408.