chore(sqllab): refactor addQueryEditor for new tab #21711
Conversation
justinpark
force-pushed
the
chore--refactor-new-query-tab-name
branch
from
24741cd
to
14cde76
Compare
justinpark
force-pushed
the
chore--refactor-new-query-tab-name
branch
from
14cde76
to
25706ae
Compare
Codecov Report
@@ Coverage Diff @@
## master #21711 +/- ##
=======================================
Coverage 66.78% 66.78%
=======================================
Files 1799 1799
Lines 68881 68882 +1
Branches 7319 7317 -2
=======================================
+ Hits 46002 46005 +3
+ Misses 20992 20991 -1
+ Partials 1887 1886 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
LGTM! |
| expect( | ||
| wrapper.instance().props.actions.addQueryEditor.getCall(0).args[0].name, | ||
| ).toContain(newTitle); | ||
| it('should properly increment query tab name', async () => { |
There was a problem hiding this 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.
There was a problem hiding this comment.
Good suggestion. However, it's a little complicated to setup entire SqlLab redux state to simulate the behavior in the screen. It'd be better to minimize the redux usage in the future for better screen testing environment.
)" This reverts commit 0c46149.
mistercrunch
added
🏷️ bot
SUMMARY
In order to get
newQueryTabName, the root component must retrieve thequeryEditorslist.This value is unnecessary during the rendering time but it only needs when the action is invoked.
This commit refactors this part that uses
queryEditorsin the action to remove this unnecessary prop in the rendering component.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
go to sqllab
create new tab and check the untitled name incremental by tab numbers
ADDITIONAL INFORMATION
cc: @ktmud @EugeneTorap