Skip to content

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Jul 28, 2025

Previously, running pytest with docker-compose-light.yml was complicated:

  • No test database setup
  • Required manual database creation and schema setup
  • Had to manage Redis dependencies (which docker-compose-light doesn't include)

This adds a pytest-runner service that makes testing seamless:

  • Zero configuration: Automatically creates test database on first run
  • Fast subsequent runs: Reuses existing test environment (~2-3 seconds startup)
  • No Redis required: Uses SimpleCache for all caching needs
  • Force reload option: Reset test database when needed with FORCE_RELOAD=true

Usage:

docker-compose -f docker-compose-light.yml run --rm pytest-runner pytest tests/unit_tests/ 

docker-compose -f docker-compose-light.yml run --rm -e FORCE_RELOAD=true pytest-runner pytest tests/

🤖 Generated with Claude Code with TLC from @mistercrunch

@dosubot dosubot bot added the install:docker Installation - docker container label Jul 28, 2025
Copy link

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Status
Security Hardcoded Database Credentials ▹ view 🧠 Not in standard
Error Handling Masked initialization failures ▹ view 🧠 Not in scope
Design Inline Python Code Duplication ▹ view 🧠 Not in standard
Readability Unused Variable ▹ view 🧠 Incorrect
Files scanned
File Path Reviewed
docker/docker-pytest-entrypoint.sh

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

Comment on lines 101 to 103
superset db upgrade 2>/dev/null || true
superset init 2>/dev/null || true
superset load-test-users 2>/dev/null || true

This comment was marked as resolved.

from psycopg2.extensions import ISOLATION_LEVEL_AUTOCOMMIT

# Connect to default database
conn = psycopg2.connect(host='db-light', user='superset', password='superset', database='superset_light')

This comment was marked as resolved.

@@ -61205,8 +61205,10 @@
},
"peerDependencies": {
"@ant-design/icons": "^5.2.6",
"@reduxjs/toolkit": "*",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk been getting lots of this lately, checkout master, start docker-compose and you'll get some of these. Guessing it's some flakiness depending on the node/npm env used to push the latest package-lock.json I think it doesn't matter one way or another ...

@geido
Copy link
Member

geido commented Jul 29, 2025

Just tried running both commands and I have got a bunch of test failures that seem to be related to the setup

FAILED tests/unit_tests/commands/report/execute_test.py::test_get_dashboard_urls_with_multiple_tabs[anchors0-permalink_side_effect0-expected_uris0] - AssertionError: assert ['http://supe...oard/p/url2/'] == ['http://0.0....oard/p/url2/']
FAILED tests/unit_tests/commands/report/execute_test.py::test_get_dashboard_urls_with_multiple_tabs[mock_tab_anchor_1-permalink_side_effect1-expected_uris1] - AssertionError: assert ['http://supe...oard/p/url1/'] == ['http://0.0....oard/p/url1/']
FAILED tests/unit_tests/commands/report/execute_test.py::test_get_dashboard_urls_with_exporting_dashboard_only - AssertionError: assert 'http://0.0.0...board/p/url1/' == 'http://super...board/p/url1/'
FAILED tests/unit_tests/commands/report/execute_test.py::test_get_tab_urls - AssertionError: assert ['http://supe...oard/p/uri2/'] == ['http://0.0....oard/p/uri2/']
FAILED tests/unit_tests/commands/report/execute_test.py::test_get_tab_url - AssertionError: assert 'http://super...hboard/p/uri/' == 'http://0.0.0...hboard/p/uri/'
FAILED tests/unit_tests/datasets/commands/importers/v1/import_test.py::test_import_column_allowed_data_url - sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) no such table: my_table
FAILED tests/unit_tests/db_engine_specs/test_gsheets.py::test_validate_parameters_simple - ModuleNotFoundError: No module named 'pip'
FAILED tests/unit_tests/db_engine_specs/test_gsheets.py::test_validate_parameters_no_catalog - ModuleNotFoundError: No module named 'pip'
FAILED tests/unit_tests/db_engine_specs/test_gsheets.py::test_validate_parameters_simple_with_in_root_catalog - ModuleNotFoundError: No module named 'pip'
FAILED tests/unit_tests/db_engine_specs/test_gsheets.py::test_upload_new - AttributeError: module 'shillelagh.backends.apsw.dialects' has no attribute 'base'
FAILED tests/unit_tests/db_engine_specs/test_gsheets.py::test_upload_existing - AttributeError: module 'shillelagh.backends.apsw.dialects' has no attribute 'base'
ERROR tests/unit_tests/db_engine_specs/test_trino.py::test_get_extra_params[extra0-expected0] - SystemExit: 1
ERROR tests/unit_tests/db_engine_specs/test_trino.py::test_get_extra_params[extra1-expected1] - SystemExit: 1
ERROR tests/unit_tests/db_engine_specs/test_trino.py::test_get_extra_params_with_server_cert - SystemExit: 1

@mistercrunch
Copy link
Member Author

Oh good catch, I didn't try to run the entire suite, but seems like a virtuous goal!

@mistercrunch
Copy link
Member Author

Some issues related to tests not being idempotent, fixing that here too

@geido
Copy link
Member

geido commented Aug 4, 2025

Still running into issues, but it might be something wrong with my machine. Maybe @betodealmeida or @Antonio-RiveroMartnez can help to confirm whether this works for them

@Antonio-RiveroMartnez
Copy link
Member

Antonio-RiveroMartnez commented Aug 5, 2025

This looks great! I executed both commands and they are FAST and just work! (1min running the whole suite)

docker-compose -f docker-compose-light.yml run --rm pytest-runner pytest tests/unit_tests/

Screen.Recording.2025-08-05.at.4.48.32.PM.mov

docker-compose -f docker-compose-light.yml run --rm -e FORCE_RELOAD=true pytest-runner pytest tests/

Screen.Recording.2025-08-05.at.4.46.37.PM.mov

Copy link
Member

@Antonio-RiveroMartnez Antonio-RiveroMartnez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@pull-request-size pull-request-size bot added size/XL and removed size/L labels Aug 5, 2025
mistercrunch added a commit that referenced this pull request Aug 5, 2025
- Fix Flask request context issues in test_core.py by properly mocking request object
- Add Shillelagh dialect skip logic for tests requiring superset:// and gsheets://
- Fix test isolation in manager_test.py by checking for existing roles/users
- All tests now pass: 2073 passed, 31 skipped, 0 failed

These fixes ensure the pytest-runner works reliably in Docker environment for PR #34373
mistercrunch and others added 8 commits August 5, 2025 17:14
Previously, running pytest with docker-compose-light.yml was complicated:
- No test database setup
- Required manual database creation and schema setup
- Had to manage Redis dependencies (which docker-compose-light doesn't include)

This adds a pytest-runner service that makes testing seamless:
- Zero configuration: Automatically creates test database on first run
- Fast subsequent runs: Reuses existing test environment (~2-3 seconds startup)
- No Redis required: Uses SimpleCache for all caching needs
- Force reload option: Reset test database when needed with FORCE_RELOAD=true

Usage:
  docker-compose -f docker-compose-light.yml run --rm pytest-runner pytest tests/unit_tests/
  docker-compose -f docker-compose-light.yml run --rm -e FORCE_RELOAD=true pytest-runner pytest tests/

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Increase PostgreSQL max_connections to 200 to handle test suite load
- Skip gsheets tests when pip module is not available (shillelagh dependency)
- Mock data loading in dataset import test to avoid connection issues
- Add pip to development requirements for test compatibility

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
The test already has a mock for urlopen via decorator, so the additional
mock for load_data was causing issues in CI.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
…ss check

- Add database readiness check with 30-second timeout to handle fresh container starts
- Always run database migrations to ensure schema is up-to-date
- Remove error suppression (2>/dev/null) for better debugging
- Add helpful tip about FORCE_RELOAD for users
- Ensure proper test database initialization from cold start

This fixes issues where tests would fail when starting from a completely fresh
state with no running containers.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Skip tests that use superset:// SQLAlchemy dialect when it's not available
- Fix security_manager patch paths to use correct import location
- Add proper error handling for Shillelagh dialect in Docker environment

This ensures tests don't fail in environments where the custom superset://
dialect can't be loaded (common in Docker containers).

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Copy tag_latest_release.sh script to Docker environment during initialization
- Enable tag_latest_release_test.py tests to run successfully in Docker context
- Script includes proper TEST_ENV mode for test execution

This fixes the 10 failing tests in tests/unit_tests/scripts/tag_latest_release_test.py
by ensuring the required bash script is available in the Docker container.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Fix Flask request context issues in test_core.py by properly mocking request object
- Add Shillelagh dialect skip logic for tests requiring superset:// and gsheets://
- Fix test isolation in manager_test.py by checking for existing roles/users
- All tests now pass: 2073 passed, 31 skipped, 0 failed

These fixes ensure the pytest-runner works reliably in Docker environment for PR #34373
mistercrunch and others added 4 commits August 5, 2025 23:14
- Add Flask g.user mocking to all test functions to fix AttributeError in CI
- Fix missing SQLALCHEMY_CUSTOM_PASSWORD_STORE config key in test_superset_limit
- Install pip module in Docker environment for Shillelagh compatibility
- Tests now run consistently in both Docker and CI environments

The tests were skipping in Docker due to missing pip module (Shillelagh dependency)
but failing in CI with missing Flask user context. This commit addresses both issues.
- Patch security_manager in superset.extensions.metadb where it's actually used
- Remove duplicate config patching in test_superset_limit (decorator handles it)
- All 6 tests in test_sqlalchemy.py now pass successfully
- Add direct g.user assignment for Flask context mocking (Python 3.8+ compatible)
- Install pip module in Docker environment for Shillelagh compatibility
- Fix config patching for test_superset_limit using @with_config decorator
- Ensure g.user exists in Flask context to prevent AttributeError in CI

All 6 tests in test_sqlalchemy.py now pass in both Docker and CI environments.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
… module

- Change from mocking generic security_manager to specifically mocking raise_for_access
- Mock at the exact import location: superset.extensions.metadb.security_manager.raise_for_access
- This ensures the mock is applied where the function is actually called
- Fixes CI failures where security checks were not being bypassed

All 6 tests in test_sqlalchemy.py now pass in both Docker and CI environments.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@mistercrunch
Copy link
Member Author

NICE -> whole suite passed twice in a row, meaning I fixed all non-idempotent tests:

Screenshot 2025-08-06 at 12 14 44 AM

This enables docker-backed reproducible results, can run the whole suite with a single, simple command: claudette pytest -x tests/. Works nicely with https://github.com/mistercrunch/claudette-cli

@mistercrunch mistercrunch merged commit 246181a into master Aug 6, 2025
48 checks passed
@mistercrunch mistercrunch deleted the docker-pytest branch August 6, 2025 04:17
LisaHusband pushed a commit to LisaHusband/superset that referenced this pull request Aug 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
install:docker Installation - docker container preset-io size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants