You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2025/01/10 20:25:39 UTC

[PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

mistercrunch opened a new pull request, #31796:
URL: https://github.com/apache/superset/pull/31796

   Running Cypress tests as depicted in docs is difficult and error-prone. Some of the challenges include:
   - having a reproducible deterministic setup (version of python that matches CI)
   - switching to a different `superset_config`
   - requires a local postgres
   - database state from previous branch can interfere in and out of a cypress setup (different set of examples, database migrations might be in a different checkpoint, ...)
   - switching to a different port
   - having celery workers working
   - caching is nice to have to accelerate tests, which may or may not be set up
   - ...
   
   Given this, I thought I'd just make it such that you can just `CYPRESS_CONFIG=true docker compose up` to fire up a backend against which you can run cypress tests locally. Given the reference to `CYPRESS_CONFIG` in our docker-compose.yml and in various `docker/`-related scripts, it looks like either it was supported in the past, or someone tried to do work in this direction, but clearly isn't working today as is.
   
   To get this working here I'm:
   - setting up `CYPRESS_CONFIG=true` to point to a dedicated database schema `superset_cypress`, this gets created on Postgres first-boot, and currently will require deleting the docker volume for Postgres to take effect
   - starting up superset app to run on `8081`, which seems to be the default port assumption for Cypress runs
   - pointing to the config file at `tests/integration_tests/superset_test_config.py`, which is need for tests, eventually might want to merge this into `docker/pythonpath/superset_config.py` for simplicity. For now I conditionally import it
   - removing duplicate docs entry existing in both `docs/docs/contributing/development.mdx` and `docs/docs/contributing/howtos.mdx`
   - adjusting docs
   
   As of current commit, tests don't seem to pass, so there's probably some things I'm missing, have to dig deeper
   


-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1911866207


##########
superset-frontend/cypress-base/cypress/support/e2e.ts:
##########
@@ -26,6 +26,10 @@ require('cy-verify-downloads').addCustomCommand();
 
 // fail on console error, allow config to override individual tests
 // these exceptions are a little pile of tech debt
+//
+
+// DISABLING FOR NOW

Review Comment:
   Ok so now I know no one has used cypress locally in a while since any console error/warning including React ones fails the tests. From my understanding the reason why it works in CI is because the `npm run build` suppresses these console errors.
   
   I'm thinking we disable for now and re-enable later after cleaning up all React warnings, which is beyond the scope of this PR



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1920662959


##########
superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts:
##########
@@ -78,20 +81,15 @@ describe('Charts list', () => {
       cy.wait('@get');
     });
 
-    it('should show the newly added dashboards in a tooltip', () => {
+    it.only('should show the newly added dashboards in a tooltip', () => {

Review Comment:
   @mistercrunch actually on second thought, we should probably add a it.skip to skip the test entirely with the old code. We should leave a comment above the test to say we skipped it because mouseover does not trigger the tooltip in cypress but does do it manually. Currently the code that is removed is the meat of showing the tooltip, so if we aren't going to validate this, we probably don't need the tests to run all the steps before the validation.



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1920672176


##########
docker-compose.yml:
##########
@@ -89,6 +89,8 @@ services:
     restart: unless-stopped
     ports:
       - 8088:8088
+      # When in cypress-mode ->
+      - 8081:8081

Review Comment:
   One thing that's kind of good with using a different port, is that firing up Cypress against `docker compose up` will fail as it's looking for Superset on 8081, forcing the user to stop docker-compose and to fire it up properly `CYPRES_CONFIG=true docker-compose up`, meaning it enforces having the right backend, with the right test sets and examples loaded up, preventing some confusion around using the wrong backend.



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1911271053


##########
docker-compose.yml:
##########
@@ -89,6 +89,8 @@ services:
     restart: unless-stopped
     ports:
       - 8088:8088
+      # When in cypress-mode ->
+      - 8081:8081

Review Comment:
   NOTE: debated whether to try and open this port conditionally, but seemed impossible and researched whether there's downside in opening a port-to-nowhere in other scenarios and I seemed there's no downside. Could also just run Cypress against 8088 too if we prefer



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1920665543


##########
docker-compose.yml:
##########
@@ -89,6 +89,8 @@ services:
     restart: unless-stopped
     ports:
       - 8088:8088
+      # When in cypress-mode ->
+      - 8081:8081

Review Comment:
   yup it's super easy, I'm just unclear if we want segmentation on PORT similar to the segmentation we have at the database level. In theory would could have both a dev and cypress environment running in parallel and not conflicting on ports ... Though I don't think you can fire two docker-compose at the same time (?)



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1915880065


##########
superset-frontend/cypress-base/cypress.config.ts:
##########
@@ -70,6 +72,7 @@ export default eyesPlugin(
       },
       baseUrl: 'http://localhost:8088',
       excludeSpecPattern: [],
+      experimentalRunAllSpecs: true,

Review Comment:
   NOTE: this adds a button to "run all tests" in-context in the Cypress UI



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31796:
URL: https://github.com/apache/superset/pull/31796#issuecomment-2591533447

   @sadpandajoe , could use help here around:
   - understanding how I broke the few tests that are failing here given the content of this PR. I'm not really changing the code/logic in any way, and most tests pass now. What did i change in this PR that could have broken those few tests? or is it just semi-random/flakiness (?)
   - help fixing the tests, I'm a bit at a loss with Cypress (though I did manage to fix some intricate issues related to timing/flaky tests. It was around an async call that would trigger a react re-render and some race-condition locally, maybe cause my laptop is faster than CI?)


-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31796:
URL: https://github.com/apache/superset/pull/31796#issuecomment-2594838476

   @sadpandajoe maybe we can pair up and dig in, I have the whole interactive Cypress thing running and interactively debugging but was stuck. Also confused as to how my backend PR creates tooltip/modal issues. Seem like timing / race condition type stuff. Tried disabling animation, longer timeouts, ...


-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1920658393


##########
superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts:
##########
@@ -78,20 +81,15 @@ describe('Charts list', () => {
       cy.wait('@get');
     });
 
-    it('should show the newly added dashboards in a tooltip', () => {
+    it.only('should show the newly added dashboards in a tooltip', () => {

Review Comment:
   need to remove this only or else the other tests won't run



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #31796:
URL: https://github.com/apache/superset/pull/31796#issuecomment-2599403373

   Ok, I'm running it in a different terminal window and no error, so that was probably it. LGTM!


-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1920658617


##########
superset-frontend/cypress-base/cypress.config.ts:
##########
@@ -26,8 +26,9 @@ export default eyesPlugin(
   defineConfig({
     chromeWebSecurity: false,
     defaultCommandTimeout: 8000,
-    numTestsKeptInMemory: 0,
-    experimentalFetchPolyfill: true,
+    numTestsKeptInMemory: 1,

Review Comment:
   I'm moving now to remove local settings from being hard coded (commenting out the setting), but by setting it to `0` in CI using command line



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1911866207


##########
superset-frontend/cypress-base/cypress/support/e2e.ts:
##########
@@ -26,6 +26,10 @@ require('cy-verify-downloads').addCustomCommand();
 
 // fail on console error, allow config to override individual tests
 // these exceptions are a little pile of tech debt
+//
+
+// DISABLING FOR NOW

Review Comment:
   NOTE: Ok so now I know no one has used cypress locally in a while since any console error/warning including React ones fails the tests. From my understanding the reason why it works in CI is because the `npm run build` suppresses these console errors.
   
   I'm thinking we disable for now and re-enable later after cleaning up all React warnings, which is beyond the scope of this PR



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1911282941


##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -107,6 +108,18 @@ class CeleryConfig:
 log_level_text = os.getenv("SUPERSET_LOG_LEVEL", "INFO")
 LOG_LEVEL = getattr(logging, log_level_text.upper(), logging.INFO)
 
+if os.getenv("CYPRESS_CONFIG") == "true":
+    # When running the service as a cypress backend, we need to import the config
+    # located @ tests/integration_tests/superset_test_config.py
+    base_dir = os.path.dirname(__file__)
+    module_folder = os.path.abspath(
+        os.path.join(base_dir, "../../tests/integration_tests/")

Review Comment:
   NOTE: conditionally loading config located at `tests/integration_tests/`, but would much rather deprecate that file and merge non-conflicting/duplicated configs directly in here. Doing this would need adjustments on CI scripts and could break local envs (probably ok as we would instruct devs to work in `docker compose`)



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1920489348


##########
superset-frontend/cypress-base/cypress.config.ts:
##########
@@ -26,8 +26,9 @@ export default eyesPlugin(
   defineConfig({
     chromeWebSecurity: false,
     defaultCommandTimeout: 8000,
-    numTestsKeptInMemory: 0,
-    experimentalFetchPolyfill: true,
+    numTestsKeptInMemory: 1,

Review Comment:
   ooops that leaked through... how about we force to `--config numTestsKeptInMemory=0` in CI, but remove the setting to use whatever the default is locally?



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1919116368


##########
docker/docker-entrypoint-initdb.d/cypress-init.sh:
##########
@@ -0,0 +1,28 @@
+#!/usr/bin/env bash
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# ------------------------------------------------------------------------
+# Creates the examples database and respective user. This database location
+# and access credentials are defined on the environment variables
+# ------------------------------------------------------------------------
+set -e
+
+psql -v ON_ERROR_STOP=1 --username "${POSTGRES_USER}" <<-EOSQL

Review Comment:
   ### Insufficient psql error feedback <sub>![category Error Handling](https://img.shields.io/badge/Error%20Handling-ea580c)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The script relies on set -e for error handling but doesn't capture or log potential psql errors meaningfully
   
   ###### Why this matters
   While ON_ERROR_STOP=1 ensures psql fails on error and set -e exits the script, there's no feedback to help diagnose what went wrong if the database creation fails
   
   ###### Suggested change ∙ *Feature Preview*
   Wrap the psql command in error handling to provide context:
   ```bash
   if ! psql -v ON_ERROR_STOP=1 --username "${POSTGRES_USER}" <<-EOSQL
     CREATE DATABASE superset_cypress;
   EOSQL
   then
     echo "Failed to create superset_cypress database" >&2
     exit 1
   fi
   ```
   
   
   </details>
   
   ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
   
   
   <!--- korbi internal id:f6f982fd-4a8b-41d4-b83b-0db3f2216131 -->
   



##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -107,6 +108,18 @@ class CeleryConfig:
 log_level_text = os.getenv("SUPERSET_LOG_LEVEL", "INFO")
 LOG_LEVEL = getattr(logging, log_level_text.upper(), logging.INFO)
 
+if os.getenv("CYPRESS_CONFIG") == "true":
+    # When running the service as a cypress backend, we need to import the config
+    # located @ tests/integration_tests/superset_test_config.py
+    base_dir = os.path.dirname(__file__)
+    module_folder = os.path.abspath(
+        os.path.join(base_dir, "../../tests/integration_tests/")
+    )
+    sys.path.insert(0, module_folder)
+    from superset_test_config import *  # noqa

Review Comment:
   ### Avoid using relative and wildcard imports. <sub>![category Security](https://img.shields.io/badge/Security-e11d48)</sub>
   
   <details>
     <summary>Tell me more</summary>
   &#8203;
   
   The code is importing the 'superset_test_config' module using a relative path and executing its contents with a wildcard import ('from superset_test_config import *'). This can be a security risk as it allows the imported file to execute arbitrary code in the application's context. To mitigate this, use an absolute import path instead of a relative path, and import only the specific names needed instead of using a wildcard import. Validate the imported names to ensure they don't contain malicious code.
   
   
   </details>
   
   ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
   
   
   <!--- korbi internal id:545129e9-1741-4a4c-af65-be2ce9d28a27 -->
   



##########
docker/docker-entrypoint-initdb.d/cypress-init.sh:
##########
@@ -0,0 +1,28 @@
+#!/usr/bin/env bash
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# ------------------------------------------------------------------------
+# Creates the examples database and respective user. This database location
+# and access credentials are defined on the environment variables
+# ------------------------------------------------------------------------
+set -e
+
+psql -v ON_ERROR_STOP=1 --username "${POSTGRES_USER}" <<-EOSQL
+  CREATE DATABASE superset_cypress;
+EOSQL

Review Comment:
   ### Missing Database Permissions <sub>![category Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The script creates a database but doesn't grant necessary permissions to access it, which is required for Cypress tests to function properly.
   
   ###### Why this matters
   Without proper permissions, the Cypress tests will fail when attempting to connect to or interact with the database, leading to test failures.
   
   ###### Suggested change ∙ *Feature Preview*
   Add GRANT statements to provide necessary permissions for the database. Example:
   ```sql
   psql -v ON_ERROR_STOP=1 --username "${POSTGRES_USER}" <<-EOSQL
     CREATE DATABASE superset_cypress;
     GRANT ALL PRIVILEGES ON DATABASE superset_cypress TO "${POSTGRES_USER}";
   EOSQL
   ```
   
   
   </details>
   
   ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
   
   
   <!--- korbi internal id:2322dd80-f469-4f84-8a31-6dae664e5323 -->
   



##########
superset-frontend/cypress-base/cypress/e2e/explore/utils.ts:
##########
@@ -68,7 +72,10 @@ export function saveChartToDashboard(dashboardName: string) {
   interceptUpdate();
   interceptExploreGet();
 
-  cy.getBySel('query-save-button').click();
+  cy.getBySel('query-save-button')
+    .should('be.enabled')
+    .should('not.be.disabled')
+    .click();

Review Comment:
   ### Redundant Button State Assertions <sub>![category Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Redundant assertion checking for both 'be.enabled' and 'not.be.disabled' on the save button. These assertions are functionally equivalent in Cypress.
   
   ###### Why this matters
   Using redundant assertions increases test execution time and adds unnecessary complexity to the test code without providing additional verification value.
   
   ###### Suggested change ∙ *Feature Preview*
   Use only one assertion to check the button state:
   ```typescript
   cy.getBySel('query-save-button')
       .should('be.enabled')
       .click();
   ```
   
   
   </details>
   
   ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
   
   
   <!--- korbi internal id:d649f57c-efa3-48dc-b588-90783562974b -->
   



##########
docker/docker-entrypoint-initdb.d/cypress-init.sh:
##########
@@ -0,0 +1,28 @@
+#!/usr/bin/env bash
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# ------------------------------------------------------------------------
+# Creates the examples database and respective user. This database location
+# and access credentials are defined on the environment variables

Review Comment:
   ### Missing Environment Variable Validation <sub>![category Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The script comment mentions credentials being defined in environment variables, but the script doesn't validate their presence.
   
   ###### Why this matters
   If required environment variables are not set, the script will fail silently or with unclear errors, causing initialization issues.
   
   ###### Suggested change ∙ *Feature Preview*
   Add environment variable validation at the beginning of the script:
   ```bash
   if [ -z "${POSTGRES_USER}" ]; then
     echo "Error: POSTGRES_USER environment variable is not set"
     exit 1
   fi
   ```
   
   
   </details>
   
   ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
   
   
   <!--- korbi internal id:e9abfc9c-4068-4314-8040-cd02392a49f2 -->
   



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #31796:
URL: https://github.com/apache/superset/pull/31796#issuecomment-2599377548

   @mistercrunch I tested the branch with running:
   `docker compose up db` (opened a shell and logged into postgres and created the superset_cypress db)
   ` CYPRESS_CONFIG=true docker-compose up`
   tests on localhost:8081-- test the login with `admin/general`
   `cd superset-frontend/cypress-base`
   `npm ci`
   `npx cypress open --config numTestsKeptInMemory=3`
   And I'm seeing this error (it's looking for port 8081)
   ```cy.request() failed trying to load:
   
   http://localhost:8088/coverage
   
   We attempted to make an http request to this URL but the request failed without a response.
   
   We received this error at the network level:
   
   > Error: socket hang up
   
   -----------------------------------------------------------
   
   The request we sent was:
   
   Method: GET
   URL: http://localhost:8088/coverage```


-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31796:
URL: https://github.com/apache/superset/pull/31796#issuecomment-2599394623

   @eschutho you probably have an env var in our current shell pointing to 8088. 
   ```
   $ env | grep -i cypress
   $ unset CYPRESS_BASE_URL
   ```


-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1920520938


##########
superset-frontend/cypress-base/cypress.config.ts:
##########
@@ -26,8 +26,9 @@ export default eyesPlugin(
   defineConfig({
     chromeWebSecurity: false,
     defaultCommandTimeout: 8000,
-    numTestsKeptInMemory: 0,
-    experimentalFetchPolyfill: true,
+    numTestsKeptInMemory: 1,

Review Comment:
   that's fine as long as it's not on in CI. 



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1915881558


##########
superset-frontend/cypress-base/cypress/support/e2e.ts:
##########
@@ -160,7 +166,18 @@ Cypress.Commands.add('login', () => {
     url: '/login/',
     body: { username: 'admin', password: 'general' },
   }).then(response => {
-    expect(response.status).to.eq(200);
+    if (response.status === 302) {

Review Comment:
   NOTE: not sure if this is required, but at some point was getting some redirect around login, this handles redirect as not a failure



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1911866286


##########
superset-frontend/cypress-base/cypress.config.ts:
##########
@@ -27,7 +27,8 @@ export default eyesPlugin(
     chromeWebSecurity: false,
     defaultCommandTimeout: 8000,
     numTestsKeptInMemory: 0,
-    experimentalFetchPolyfill: true,
+    // Disabled after realizing this MESSES UP rison encoding in intricate ways
+    experimentalFetchPolyfill: false,

Review Comment:
   This bug was gnarly to debug. Screw this experimental feature.



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1911290632


##########
superset/config.py:
##########
@@ -1923,7 +1923,7 @@ class ExtraDynamicQueryFilters(TypedDict, total=False):
             "Failed to import config for %s=%s", CONFIG_PATH_ENV_VAR, cfg_path
         )
         raise
-elif importlib.util.find_spec("superset_config") and not is_test():

Review Comment:
   NOTE: this seemed to say "if you're in test mode, DONT load ANY custom config", which is a confusing hack and probably doesn't belong here or anywhere



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1911285902


##########
docs/docs/contributing/development.mdx:
##########
@@ -631,71 +631,6 @@ To run a single test file:
 npm run test -- path/to/file.js
 ```
 
-### Integration Testing

Review Comment:
   NOTE: these docs, and many other entries in these files are duplicated in `./howtos.mdx` . It's a bit tricky as they are not pure dups as there has been edits on both sides



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1915879775


##########
superset-frontend/cypress-base/cypress.config.ts:
##########
@@ -27,7 +27,8 @@ export default eyesPlugin(
     chromeWebSecurity: false,
     defaultCommandTimeout: 8000,
     numTestsKeptInMemory: 0,
-    experimentalFetchPolyfill: true,
+    // Disabled after realizing this MESSES UP rison encoding in intricate ways
+    experimentalFetchPolyfill: false,

Review Comment:
   Now wondering if switching this off could be what's failing the few tests (?)



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1920667000


##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -107,6 +108,18 @@ class CeleryConfig:
 log_level_text = os.getenv("SUPERSET_LOG_LEVEL", "INFO")
 LOG_LEVEL = getattr(logging, log_level_text.upper(), logging.INFO)
 
+if os.getenv("CYPRESS_CONFIG") == "true":
+    # When running the service as a cypress backend, we need to import the config
+    # located @ tests/integration_tests/superset_test_config.py
+    base_dir = os.path.dirname(__file__)
+    module_folder = os.path.abspath(
+        os.path.join(base_dir, "../../tests/integration_tests/")
+    )
+    sys.path.insert(0, module_folder)
+    from superset_test_config import *  # noqa

Review Comment:
   which part of `# noqa` don't you understand @korbit-ai !?



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1920667279


##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -107,6 +108,18 @@ class CeleryConfig:
 log_level_text = os.getenv("SUPERSET_LOG_LEVEL", "INFO")
 LOG_LEVEL = getattr(logging, log_level_text.upper(), logging.INFO)
 
+if os.getenv("CYPRESS_CONFIG") == "true":
+    # When running the service as a cypress backend, we need to import the config
+    # located @ tests/integration_tests/superset_test_config.py
+    base_dir = os.path.dirname(__file__)
+    module_folder = os.path.abspath(
+        os.path.join(base_dir, "../../tests/integration_tests/")
+    )
+    sys.path.insert(0, module_folder)
+    from superset_test_config import *  # noqa

Review Comment:
   Hi @mistercrunch, I understand your usage of `# noqa` to suppress linter warnings. However, the comment about the relative and wildcard imports isn't about linting — it's more of a security concern. While `# noqa` will keep linters from complaining, it won't address the potential security risks tied to your import. My suggestion would be to replace the wildcard import with explicit imports of the attributes you really need, and switch to absolute path when importing the `superset_test_config.py`. Appreciate your efforts!



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #31796:
URL: https://github.com/apache/superset/pull/31796#issuecomment-2593417279

   > @sadpandajoe , could use help here around:
   > 
   > * understanding how I broke the few tests that are failing here given the content of this PR. I'm not really changing the code/logic in any way, and most tests pass now. What did i change in this PR that could have broken those few tests? or is it just semi-random/flakiness (?)
   > * help fixing the tests, I'm a bit at a loss with Cypress (though I did manage to fix some intricate issues related to timing/flaky tests. It was around an async call that would trigger a react re-render and some race-condition locally, maybe cause my laptop is faster than CI?)
   
   @mistercrunch do you know if we stored the failed screenshots anywhere? If so it should have a snapshot of the failure state. It looks like the two errors are grouped as:
   - antd5 tooltip not being found which could either be the page didn't load correctly or that the element isn't antd5 yet. So having the screenshot could eliminate one of them
   - delete modal button not found. same thing here as it could be that the selector has changed or the modal never appears. 


-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1919330666


##########
docker-compose.yml:
##########
@@ -89,6 +89,8 @@ services:
     restart: unless-stopped
     ports:
       - 8088:8088
+      # When in cypress-mode ->
+      - 8081:8081

Review Comment:
   Is it fairly simple to run Cypress against 8088? I know that we can change the port when running locally. 



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1919364897


##########
docs/docs/contributing/howtos.mdx:
##########
@@ -225,22 +225,10 @@ npm run test -- path/to/file.js
 
 ### e2e Integration Testing
 
-For e2e testing, we recommend that you use a `docker-compose` backed-setup
-
-Alternatively, you can go lower level and set things up in your
-development environment by following these steps:
-
-First set up a python/flask backend:
+For e2e testing, we recommend that you use a `docker compose` backend
 
 ```bash
-export SUPERSET_CONFIG=tests.integration_tests.superset_test_config
-export SUPERSET_TESTENV=true
-export CYPRESS_BASE_URL="http://localhost:8081"
-superset db upgrade
-superset load_test_users
-superset init
-superset load-examples --load-test-data
-superset run --port 8081
+CYPRESS_CONFIG=true docker compose up

Review Comment:
   Will this also load all the assets? I had an issue so working from a clean state and it looks like charts/dashboards, didn't load for me at all when spinning this up with this command.



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1911277525


##########
docker/docker-entrypoint-initdb.d/cypress-init.sh:
##########
@@ -0,0 +1,28 @@
+#!/usr/bin/env bash
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# ------------------------------------------------------------------------
+# Creates the examples database and respective user. This database location
+# and access credentials are defined on the environment variables
+# ------------------------------------------------------------------------
+set -e
+
+psql -v ON_ERROR_STOP=1 --username "${POSTGRES_USER}" <<-EOSQL
+  CREATE DATABASE superset_cypress;

Review Comment:
   NOTE: this script runs only on first-init of the postgres image, so will require devs to either delete their postgres-supporting volume (one easy command) or to shell into `psql` and `CREATE DATABASE`. Considering adding a `NUKE_DATABASE` flag more broadly in `docker-compose` too



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #31796:
URL: https://github.com/apache/superset/pull/31796#issuecomment-2584015118

   Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
   
   > Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
   
   


-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1915880771


##########
superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts:
##########
@@ -49,8 +50,10 @@ function confirmDelete() {
 
 function visitChartList() {
   interceptFiltering();
+  interceptFavoriteStatus();
   cy.visit(CHART_LIST);
   cy.wait('@filtering');
+  cy.wait('@favoriteStatus');

Review Comment:
   without this `cy.wait` locally (not in CI), during the test assertion the dom would get re-rendered and I'd get some error about "elements being disconnected from the dom" or something to that effect. 



##########
superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts:
##########
@@ -49,8 +50,10 @@ function confirmDelete() {
 
 function visitChartList() {
   interceptFiltering();
+  interceptFavoriteStatus();
   cy.visit(CHART_LIST);
   cy.wait('@filtering');
+  cy.wait('@favoriteStatus');

Review Comment:
   NOTE: without this `cy.wait` locally (not in CI), during the test assertion the dom would get re-rendered and I'd get some error about "elements being disconnected from the dom" or something to that effect. 



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1920667778


##########
docs/docs/contributing/howtos.mdx:
##########
@@ -225,22 +225,10 @@ npm run test -- path/to/file.js
 
 ### e2e Integration Testing
 
-For e2e testing, we recommend that you use a `docker-compose` backed-setup
-
-Alternatively, you can go lower level and set things up in your
-development environment by following these steps:
-
-First set up a python/flask backend:
+For e2e testing, we recommend that you use a `docker compose` backend
 
 ```bash
-export SUPERSET_CONFIG=tests.integration_tests.superset_test_config
-export SUPERSET_TESTENV=true
-export CYPRESS_BASE_URL="http://localhost:8081"
-superset db upgrade
-superset load_test_users
-superset init
-superset load-examples --load-test-data
-superset run --port 8081
+CYPRESS_CONFIG=true docker compose up

Review Comment:
   yes it will, that's somewhere in the docker init file



-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #31796:
URL: https://github.com/apache/superset/pull/31796


-- 
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: notifications-unsubscribe@superset.apache.org

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

Re: [PR] feat: get docker-compose to work as the backend for Cypress tests [superset]

Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #31796:
URL: https://github.com/apache/superset/pull/31796#discussion_r1919115828


##########
superset-frontend/cypress-base/cypress.config.ts:
##########
@@ -26,8 +26,9 @@ export default eyesPlugin(
   defineConfig({
     chromeWebSecurity: false,
     defaultCommandTimeout: 8000,
-    numTestsKeptInMemory: 0,
-    experimentalFetchPolyfill: true,
+    numTestsKeptInMemory: 1,

Review Comment:
   you want this to be 0 for ci, so i'd see if this could be overridden as part of the command to start up cypress



-- 
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: notifications-unsubscribe@superset.apache.org

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