You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/09/21 07:42:09 UTC
[GitHub] [incubator-superset] adam-stasiak opened a new pull request #10973: chore: cypress selectors refactor in explore module
adam-stasiak opened a new pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR provides selectors refactor for most tests in explore test module (without visualization). I skipped some changes related to react-select components because it needs to be organized differently than normal components.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10973: chore: cypress selectors refactor in explore module
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973#discussion_r498003732
##########
File path: superset-frontend/src/explore/components/AdhocFilterEditPopover.jsx
##########
@@ -176,6 +181,7 @@ export default class AdhocFilterEditPopover extends React.Component {
</Tabs>
<div>
<Button
+ data-test="modal-save-button"
Review comment:
this isn't a modal, but rather a popover. That said, I think again that it might be wise to be specific, e.g. `save-adhoc-filter-button`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10973: chore: cypress selectors refactor in explore module
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973#discussion_r497997662
##########
File path: superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
##########
@@ -70,12 +72,16 @@ describe('AdhocMetrics', () => {
cy.get('[data-test=metrics]').find('.option-label').last().click();
// add custom SQL
- cy.get('#adhoc-metric-edit-tabs-tab-SQL').click();
- cy.get('#metrics-edit-popover').find('.ace_content').click();
- cy.get('#metrics-edit-popover')
+ cy.get('[data-test="adhoc-metric-edit-tabs"]')
+ .contains('Custom SQL')
+ .click();
+ cy.get('[data-test="metrics-edit-popover"]')
.find('.ace_text-input')
.type('/COUNT(DISTINCT name)', { force: true });
- cy.get('#metrics-edit-popover').find('button').contains('Save').click();
+ cy.get('[data-test="metrics-edit-popover"]')
+ .find('[data-test="AdhocMetricEdit#save"]')
+ .contains('Save')
Review comment:
Also wondering if we need the `get`->`find` chain here, or if we can just `get` the specific element we want to test on, with a specific enough attribute
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10973: chore: cypress selectors refactor in explore module
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973#discussion_r497997177
##########
File path: superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts
##########
@@ -77,12 +77,17 @@ describe('AdhocFilters', () => {
cy.wait('@filterValues');
- cy.get('#filter-edit-popover #adhoc-filter-edit-tabs-tab-SQL').click();
- cy.get('#filter-edit-popover .ace_content').click();
- cy.get('#filter-edit-popover .ace_text-input').type(
- "'Amy' OR name = 'Bob'",
- );
- cy.get('#filter-edit-popover button').contains('Save').click();
+ cy.get('[data-test="filter-edit-popover"]')
+ .find('[data-test="adhoc-filter-edit-tabs"]')
+ .contains('Custom SQL')
Review comment:
Curious if you think we need to do the get->find->contains->click work here, or if we should just make an explicit/specific attribute here and use it directly, e.g. `cy.get('[data-test="adhoc-filter-edit-tab-custom-sql"]').click()
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10973: chore: cypress selectors refactor in explore module
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973#discussion_r498003732
##########
File path: superset-frontend/src/explore/components/AdhocFilterEditPopover.jsx
##########
@@ -176,6 +181,7 @@ export default class AdhocFilterEditPopover extends React.Component {
</Tabs>
<div>
<Button
+ data-test="modal-save-button"
Review comment:
this isn't a modal, but rather a popover.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] eschutho commented on a change in pull request #10973: chore: cypress selectors refactor in explore module
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973#discussion_r497705951
##########
File path: superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
##########
@@ -70,12 +72,16 @@ describe('AdhocMetrics', () => {
cy.get('[data-test=metrics]').find('.option-label').last().click();
// add custom SQL
- cy.get('#adhoc-metric-edit-tabs-tab-SQL').click();
- cy.get('#metrics-edit-popover').find('.ace_content').click();
- cy.get('#metrics-edit-popover')
+ cy.get('[data-test="adhoc-metric-edit-tabs"]')
+ .contains('Custom SQL')
+ .click();
+ cy.get('[data-test="metrics-edit-popover"]')
.find('.ace_text-input')
.type('/COUNT(DISTINCT name)', { force: true });
- cy.get('#metrics-edit-popover').find('button').contains('Save').click();
+ cy.get('[data-test="metrics-edit-popover"]')
+ .find('[data-test="AdhocMetricEdit#save"]')
+ .contains('Save')
Review comment:
do we still need the lookups by text if we're using the data-test attributes?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10973: chore: cypress selectors refactor in explore module
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973#discussion_r498006407
##########
File path: superset-frontend/src/CRUD/CollectionTable.tsx
##########
@@ -292,7 +292,7 @@ export default class CRUDCollection extends React.PureComponent<
</Button>
)}
</span>
- <table className="table">
+ <table data-test="table" className="table">
Review comment:
maybe `crud-table` just to make sure we don't find ourselves running into an issue with two tables conflicting on a page? Sorry, I know these problems all seem hypothetical, but this is an excuse to _not_ run into the same issues we do with class names!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10973: chore: cypress selectors refactor in explore module
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973#discussion_r498005826
##########
File path: superset-frontend/src/chart/Chart.jsx
##########
@@ -201,7 +201,10 @@ class Chart extends React.PureComponent {
showMessage={false}
>
<Styles className="chart-container">
- <div className={`slice_container ${isFaded ? ' faded' : ''}`}>
+ <div
+ className={`slice_container ${isFaded ? ' faded' : ''}`}
Review comment:
There are probably a million of these in the codebase... wonder if it's worth tasking someone to go on a seek and destroy mission throughout the codebase, to better establish the pattern.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] adam-stasiak closed pull request #10973: chore: cypress selectors refactor in explore module
Posted by GitBox <gi...@apache.org>.
adam-stasiak closed pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10973: chore: cypress selectors refactor in explore module
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973#discussion_r498002989
##########
File path: superset-frontend/src/explore/components/QueryAndSaveBtns.jsx
##########
@@ -115,6 +115,7 @@ export default function QueryAndSaveBtns({
data-toggle="modal"
disabled={saveButtonDisabled}
onClick={onSave}
+ data-test="save-button"
Review comment:
Wondering if something more-explicit, e.g. `explore-save-button` might prevent a future issue where we have two `save-button` elements, like this one and some other Save button in a Popover or Modal.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10973: chore: cypress selectors refactor in explore module
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973#discussion_r497997177
##########
File path: superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts
##########
@@ -77,12 +77,17 @@ describe('AdhocFilters', () => {
cy.wait('@filterValues');
- cy.get('#filter-edit-popover #adhoc-filter-edit-tabs-tab-SQL').click();
- cy.get('#filter-edit-popover .ace_content').click();
- cy.get('#filter-edit-popover .ace_text-input').type(
- "'Amy' OR name = 'Bob'",
- );
- cy.get('#filter-edit-popover button').contains('Save').click();
+ cy.get('[data-test="filter-edit-popover"]')
+ .find('[data-test="adhoc-filter-edit-tabs"]')
+ .contains('Custom SQL')
Review comment:
Curious if you think we need to do the get->find->contains->click work here, or if we should just make an explicit/specific attribute here and use it directly, e.g. `cy.get('[data-test="adhoc-filter-edit-tab-custom-sql"]).click()`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] eschutho commented on a change in pull request #10973: chore: cypress selectors refactor in explore module
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #10973:
URL: https://github.com/apache/incubator-superset/pull/10973#discussion_r497707882
##########
File path: superset-frontend/src/chart/Chart.jsx
##########
@@ -201,7 +201,10 @@ class Chart extends React.PureComponent {
showMessage={false}
>
<Styles className="chart-container">
- <div className={`slice_container ${isFaded ? ' faded' : ''}`}>
+ <div
+ className={`slice_container ${isFaded ? ' faded' : ''}`}
Review comment:
this is old code, but maybe we can refactor this with `classnames` (cx)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org