michael-s-molina commented on code in PR #21711:
URL: https://github.com/apache/superset/pull/21711#discussion_r990119988
##########
superset-frontend/src/SqlLab/components/TabbedSqlEditors/TabbedSqlEditors.test.jsx:
##########
@@ -163,23 +169,32 @@ describe('TabbedSqlEditors', () => {
wrapper.instance().props.actions.removeQueryEditor.getCall(0).args[0],
).toBe(queryEditors[0]);
});
- it('should add new query editor', () => {
- wrapper = getWrapper();
- sinon.stub(wrapper.instance().props.actions, 'addQueryEditor');
-
- wrapper.instance().newQueryEditor();
- expect(
- wrapper.instance().props.actions.addQueryEditor.getCall(0).args[0].name,
- ).toContain('Untitled Query');
+ it('should add new query editor', async () => {
+ const { getAllByLabelText } = setup(mockedProps);
+ fireEvent.click(getAllByLabelText('Add tab')[0]);
+ const actions = store.getActions();
+ await waitFor(() =>
+ expect(actions).toContainEqual({
+ type: 'ADD_QUERY_EDITOR',
+ queryEditor: expect.objectContaining({
+ name: expect.stringMatching(/Untitled Query (\d+)+/),
+ }),
+ }),
+ );
});
- it('should properly increment query tab name', () => {
- wrapper = getWrapper();
- sinon.stub(wrapper.instance().props.actions, 'addQueryEditor');
- const newTitle = newQueryTabName(wrapper.instance().props.queryEditors);
- wrapper.instance().newQueryEditor();
- expect(
- wrapper.instance().props.actions.addQueryEditor.getCall(0).args[0].name,
- ).toContain(newTitle);
+ it('should properly increment query tab name', async () => {
Review Comment:
RTL best practices recommend testing what the user interacts with instead of
implementation details. Here's a quote from their site:
> You want to write maintainable tests that give you high confidence that
your components are working for your users. As a part of this goal, you want
your tests to avoid including implementation details so refactors of your
components (changes to implementation but not functionality) don’t break your
tests and slow you and your team down.
In this case, it would be better if we test what appears on the screen like
checking if there's a new tab with the right title. Even if we change the
action name or other logic in the future, the expected result would be the
same. This is not a blocker for me though, it's just a suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
