chore: Cypress all selectors refactor #11134

adam-stasiak · 2020-10-01T22:54:46Z

SUMMARY

This PR contains merged changes from outddated but reviewed branches. I resolved most of accuracy issues included in review notes. Not everywhere it was possible to add data-test selector to make some cypress steps more specific.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-commenter · 2020-10-05T12:08:59Z

Codecov Report

Merging #11134 into master will decrease coverage by 4.54%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11134      +/-   ##
==========================================
- Coverage   65.88%   61.33%   -4.55%     
==========================================
  Files         827      391     -436     
  Lines       39046    24483   -14563     
  Branches     3673        0    -3673     
==========================================
- Hits        25725    15017   -10708     
+ Misses      13214     9466    -3748     
+ Partials      107        0     -107

Flag	Coverage Δ
#cypress	`?`
#javascript	`?`
#python	`61.33% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/db_engine_specs/presto.py	`81.42% <0.00%> (-0.65%)`	⬇️
superset-frontend/src/utils/parseCookie.ts
...end/src/SqlLab/components/TemplateParamsEditor.jsx
superset-frontend/src/components/ErrorBoundary.jsx
...nd/src/dashboard/util/getDetailedComponentWidth.js
...dashboard/components/menu/MarkdownModeDropdown.jsx
superset-frontend/src/addSlice/index.tsx
superset-frontend/src/logger/actions/index.js
...src/dashboard/components/gridComponents/Header.jsx
superset-frontend/src/utils/errorMessages.js
... and 377 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6358a7f...f5bbf61. Read the comment docs.

willbarrett · 2020-10-05T22:33:45Z

@adam-stasiak this PR is huge and hard to review. Could you break this down into multiple smaller PRs?

adam-stasiak · 2020-10-05T22:38:36Z

@willbarrett I did this big PR because my partials were outdated and it was hard to rebase them when I was adding module by module.

I can split this by Cypress module fraction so split into PR for Dashboard, PR for Explore module, but I am afraid it would be hard to seperate changes in components. Is it ok to just make this smaller with cypress part not TSX?

willbarrett · 2020-10-05T22:40:16Z

@rusackas do you have any suggestions here?

rusackas · 2020-10-07T05:43:13Z

superset-frontend/cypress-base/cypress/integration/explore/visualizations/pivot_table.test.js

  });

-  it('should work with multiple groupby and multiple metrics', () => {
+  it.only('should work with multiple groupby and multiple metrics', () => {


I don't believe we want the .only here, right?

rusackas · 2020-10-07T06:02:07Z

superset-frontend/src/common/components/Modal.tsx

@@ -78,11 +78,6 @@ const StyledModal = styled(BaseModal)`
      margin-left: 8px;
    }
  }
-
-  // styling for Tabs component


Curious if/how this is relevant to the PR... looks like it would be a visual change... is it something @kgabryje is aware of?

It looks like a rebasing mistake to me. Adam, can you please bring it back?

superset-frontend/src/components/ListView/TableCollection.tsx

rusackas · 2020-10-07T06:18:14Z

superset-frontend/src/components/ListView/TableCollection.tsx

@@ -271,15 +276,16 @@ export default function TableCollection({
                  const columnCellProps = cell.column.cellProps || {};
                  return (
                    <td
-                      className={cx('table-cell', {
+                      data-test="row-cell"
+                      className={cx('row-cell', {


The table-cell class removed here appears to have CSS styles that were applied to it. Is there a reason to cahge it?

rusackas · 2020-10-07T17:30:13Z

@adam-stasiak this PR is huge and hard to review. Could you break this down into multiple smaller PRs?

I'm just slogging through it, a few files here and a few files there, making comments as I go. Looks like it needs a rebase currently, but I'll keep revisiting, and we can just shove it across the line.

adam-stasiak · 2020-10-07T17:33:08Z

@adam-stasiak this PR is huge and hard to review. Could you break this down into multiple smaller PRs?

I'm just slogging through it, a few files here and a few files there, making comments as I go. Looks like it needs a rebase currently, but I'll keep revisiting, and we can just shove it across the line.

How would you split this? I would vote for PR for TSX changes and then PRs for cypress modules?

rusackas · 2020-10-07T20:20:45Z

superset-frontend/src/dashboard/components/IconButton.jsx

      >
-        <span className={className} />
+        <span {...{ 'data-test': 'icon-button-span' }} className={className} />


Curious why the spread syntax here instead of the usual attribute.

rusackas · 2020-10-07T20:40:30Z

superset-frontend/src/explore/components/ExploreChartHeader.jsx

+      <StyledHeader
+        id="slice-header"
+        className="clearfix panel-title-large"
+        ref


Why the 'ref' prop?

rusackas · 2020-10-07T20:42:14Z

superset-frontend/src/explore/components/PropertiesModal.tsx

@@ -159,7 +159,7 @@ function PropertiesModal({ slice, onHide, onSave }: InternalProps) {

  return (
    <form onSubmit={onSubmit}>
-      <Modal.Header closeButton>
+      <Modal.Header data-test="edit-modal" closeButton>


Could be more specific (i.e. 'edit-properties-modal') if you can expect there to be more than one instance of modal in the code with data-test="edit-modal" - it just makes it easier to find the instance in the codebase.

rusackas · 2020-10-07T20:45:22Z

superset-frontend/src/explore/components/controls/DateFilterControl.jsx

                      <div style={{ margin: '5px 0' }}>
                        <Datetime
+                          inputProps={{ 'data-test': 'date-from-input' }}


Hmm... I wonder if that babel extension will manage strip out data-test attributes added like this. ¯\_(ツ)_/¯

rusackas · 2020-10-07T20:47:03Z

superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx

@@ -293,7 +293,10 @@ function DashboardList(props: DashboardListProps) {
                      className="action-button"
                      onClick={confirmDelete}
                    >
-                      <Icon name="trash" />
+                      <Icon
+                        {...{ 'data-test': 'dashboard-list-trash-icon' }}


Wondering again about the need spread syntax over the attribute, which could be stripped out by babel for sure.

rusackas · 2020-10-07T20:47:24Z

superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx

@@ -432,7 +435,11 @@ function DashboardList(props: DashboardListProps) {
                  className="action-button"
                  onClick={confirmDelete}
                >
-                  <ListViewCard.MenuIcon name="trash" /> Delete
+                  <ListViewCard.MenuIcon
+                    {...{ 'data-test': 'dashboard-card-view-trash-icon' }}


same spread question here :D

rusackas

Ok, I went through and added lots of little questions/comments/concerns. I think this is quite close to mergeable, but would like to resolve those conversations in the thread first.

fixed port fixed tests review changes

codecov-io · 2020-10-13T14:08:12Z

Codecov Report

Merging #11134 into master will decrease coverage by 4.83%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11134      +/-   ##
==========================================
- Coverage   65.60%   60.77%   -4.84%     
==========================================
  Files         832      394     -438     
  Lines       39400    24716   -14684     
  Branches     3593        0    -3593     
==========================================
- Hits        25849    15021   -10828     
+ Misses      13442     9695    -3747     
+ Partials      109        0     -109

Flag	Coverage Δ
#cypress	`?`
#javascript	`?`
#python	`60.77% <ø> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/db_engine_specs/sqlite.py	`65.62% <0.00%> (-9.38%)`	⬇️
superset/utils/celery.py	`82.14% <0.00%> (-3.58%)`	⬇️
superset/result_set.py	`96.69% <0.00%> (-1.66%)`	⬇️
...rc/explore/components/controls/ViewportControl.jsx
.../src/dashboard/components/UndoRedoKeylisteners.jsx
superset-frontend/src/common/components/Tabs.tsx
superset-frontend/src/components/Label/index.tsx
superset-frontend/src/showSavedQuery/utils.js
...rc/dashboard/components/FilterIndicatorTooltip.jsx
...-frontend/src/dashboard/actions/dashboardLayout.js
... and 382 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88af85a...dae02cb. Read the comment docs.

rusackas · 2020-10-15T17:19:25Z

@adam-stasiak should this PR be closed in favor of the newer ones?

superset-github-bot bot added the polidea label Oct 1, 2020

pull-request-size bot added the size/XXL label Oct 1, 2020

adam-stasiak force-pushed the cypress-all branch from c4e047e to 3bee7b7 Compare October 2, 2020 22:22

adam-stasiak changed the title ~~[WIP] chore: Cypress all selectors refactor~~ chore: Cypress all selectors refactor Oct 2, 2020

adam-stasiak force-pushed the cypress-all branch from d321c5a to 35d618a Compare October 5, 2020 10:41

rusackas reviewed Oct 7, 2020

View reviewed changes

superset-frontend/src/components/ListView/TableCollection.tsx Show resolved Hide resolved

rusackas reviewed Oct 7, 2020

View reviewed changes

rusackas requested changes Oct 7, 2020

View reviewed changes

Adam Stasiak added 8 commits October 13, 2020 15:51

Cypress selectors refactor

3d44f79

fixed port fixed tests review changes

fixed review changes

20865a5

tests fixes

15fb9c9

rebase fixes

7c66b14

removed some code

19ec2eb

formatting

db95456

fixed tests

6b28e32

increased animation wait

43c6b8a

longer timeouts for higher stability

7dc243e

adam-stasiak force-pushed the cypress-all branch from b5a238f to 7dc243e Compare October 13, 2020 13:56

Adam Stasiak added 3 commits October 13, 2020 16:15

removed some artifacts from rebase

dae02cb

fixed rebase problem in Modal.tsx

c232593

review change

65078cc

adam-stasiak mentioned this pull request Oct 14, 2020

chore: Added data-test attributes for testing #11254

Merged

6 tasks

adam-stasiak closed this Oct 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Cypress all selectors refactor #11134

chore: Cypress all selectors refactor #11134

adam-stasiak commented Oct 1, 2020 •

edited

Loading

codecov-commenter commented Oct 5, 2020 •

edited

Loading

willbarrett commented Oct 5, 2020

adam-stasiak commented Oct 5, 2020

willbarrett commented Oct 5, 2020

rusackas Oct 7, 2020

rusackas Oct 7, 2020

kgabryje Oct 7, 2020

rusackas Oct 7, 2020

rusackas commented Oct 7, 2020

adam-stasiak commented Oct 7, 2020

rusackas Oct 7, 2020

rusackas Oct 7, 2020

rusackas Oct 7, 2020

rusackas Oct 7, 2020

rusackas Oct 7, 2020

rusackas Oct 7, 2020

rusackas left a comment

codecov-io commented Oct 13, 2020 •

edited

Loading

rusackas commented Oct 15, 2020

chore: Cypress all selectors refactor #11134

chore: Cypress all selectors refactor #11134

Conversation

adam-stasiak commented Oct 1, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-commenter commented Oct 5, 2020 • edited Loading

Codecov Report

willbarrett commented Oct 5, 2020

adam-stasiak commented Oct 5, 2020

willbarrett commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusackas commented Oct 7, 2020

adam-stasiak commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 13, 2020 • edited Loading

Codecov Report

rusackas commented Oct 15, 2020

adam-stasiak commented Oct 1, 2020 •

edited

Loading

codecov-commenter commented Oct 5, 2020 •

edited

Loading

codecov-io commented Oct 13, 2020 •

edited

Loading