Automatically invoke Docker when running pytest #106

kesmit13 · 2025-11-14T19:12:03Z

No description provided.

Copilot

Pull Request Overview

This PR adds automatic Docker container management for pytest tests, eliminating the need for manual container setup before running tests. The implementation introduces a conftest.py file with pytest hooks that automatically start a SingleStore Docker container if SINGLESTOREDB_URL is not set, and adds support for both MySQL protocol and HTTP Data API connections.

Key changes:

Automatic Docker container lifecycle management via pytest hooks
Support for using existing servers or auto-managed containers
HTTP Data API support with USE_DATA_API environment variable

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
singlestoredb/tests/conftest.py	New file implementing pytest hooks for automatic container setup/teardown and environment protection fixtures
singlestoredb/pytest.py	Enhanced _TestContainerManager to support existing servers, added HTTP URL property, fixed port mappings, and added output suppression for cleaner test output
singlestoredb/tests/utils.py	Updated INIT_DB_URL logic to always use MySQL protocol for setup operations and added logging
singlestoredb/tests/test_plugin.py	Added conditional test skipping for HTTP connections that don't support USE database commands
pyproject.toml	Added docker package to test dependencies

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-14T19:18:01Z

singlestoredb/pytest.py

            env = {
                'SINGLESTORE_LICENSE': license,
            }


The start() method attempts to access os.environ['SINGLESTORE_LICENSE'] directly, which will raise a KeyError if the environment variable is not set. This contradicts the earlier logic in __init__ (lines 117-121) where the code uses .get() with a fallback to an empty string.

The code should use os.environ.get('SINGLESTORE_LICENSE', '') instead to be consistent with the initialization logic and avoid a KeyError when the license is not set.

Copilot · 2025-11-14T19:18:02Z

singlestoredb/pytest.py

+            )
        except Exception as e:
            logger.exception(e)
            raise RuntimeError('Failed to stop container.') from e


The error message "Failed to stop container." is misleading because this exception handler is for the docker rm command, not the docker stop command. The message should be "Failed to remove container." to accurately reflect the operation that failed.

Suggested change

raise RuntimeError('Failed to stop container.') from e

raise RuntimeError('Failed to remove container.') from e

Copilot · 2025-11-14T19:18:02Z

singlestoredb/tests/conftest.py

+        if not _container_manager.use_existing:
+            _container_manager.start()
+            print(f'Container {_container_manager.container_name} started')
+            print('Waiting for SingleStoreDB to be ready...')
+
+            # Wait for container to be ready
+            try:
+                conn = _container_manager.connect()
+                conn.close()
+                print('✓ SingleStoreDB is ready!')
+                logger.info('Docker container is ready')
+            except Exception as e:
+                print(f'✗ Failed to connect to Docker container: {e}')
+                logger.error(f'Failed to connect to Docker container: {e}')
+                raise
+


[nitpick] The condition if not _container_manager.use_existing: on line 76 is redundant. Since line 66 already checks that SINGLESTOREDB_URL is not in the environment, and _TestContainerManager.__init__() sets use_existing based on the presence of SINGLESTOREDB_URL, the use_existing flag will always be False at this point.

This check could be removed to simplify the logic, or the outer condition on line 66 could be removed to allow the _TestContainerManager to handle the logic internally. The current structure is confusing and adds unnecessary nesting.

Suggested change

if not _container_manager.use_existing:

_container_manager.start()

print(f'Container {_container_manager.container_name} started')

print('Waiting for SingleStoreDB to be ready...')

# Wait for container to be ready

try:

conn = _container_manager.connect()

conn.close()

print('✓ SingleStoreDB is ready!')

logger.info('Docker container is ready')

except Exception as e:

print(f'✗ Failed to connect to Docker container: {e}')

logger.error(f'Failed to connect to Docker container: {e}')

raise

_container_manager.start()

print(f'Container {_container_manager.container_name} started')

print('Waiting for SingleStoreDB to be ready...')

# Wait for container to be ready

try:

conn = _container_manager.connect()

conn.close()

print('✓ SingleStoreDB is ready!')

logger.info('Docker container is ready')

except Exception as e:

print(f'✗ Failed to connect to Docker container: {e}')

logger.error(f'Failed to connect to Docker container: {e}')

raise

Copilot · 2025-11-14T19:18:02Z

singlestoredb/pytest.py

+        # Use SINGLESTORE_LICENSE from environment, or empty string as fallback
+        # Empty string works for the client SDK
+        license = os.environ.get('SINGLESTORE_LICENSE', '')
+        if not license:
+            logger.info('SINGLESTORE_LICENSE not set, using empty string')


The license variable retrieved on line 119 is never actually used. Line 125 sets 'SINGLESTORE_LICENSE': None in the environment_vars dictionary, ignoring the license value that was just retrieved. This appears to be a bug.

If the intent is to use the retrieved license value, the code should be:

'SINGLESTORE_LICENSE': None if not license else license,

or simply remove the license retrieval code if it's not meant to be used here (since it's set separately in the start() method's env parameter).

Copilot · 2025-11-14T19:18:03Z

singlestoredb/tests/test_plugin.py

+    'http://' in os.environ.get('SINGLESTOREDB_URL', '') or
+    'https://' in os.environ.get('SINGLESTOREDB_URL', ''),


The skip condition checks for 'http://' or 'https://' in the URL, but the actual HTTP URL format used in conftest.py (line 149 of pytest.py) is singlestoredb+http://, not http://. This means the skip condition will fail to skip these tests when using the HTTP Data API, which could cause test failures.

The condition should be updated to:

pytestmark = pytest.mark.skipif( 'http' in os.environ.get('SINGLESTOREDB_URL', ''), reason='Plugin tests require MySQL protocol (USE database not supported via HTTP)', )

or more precisely:

pytestmark = pytest.mark.skipif( any(x in os.environ.get('SINGLESTOREDB_URL', '') for x in ['http://', 'https://', '+http://', '+https://']), reason='Plugin tests require MySQL protocol (USE database not supported via HTTP)', )

Suggested change

'http://' in os.environ.get('SINGLESTOREDB_URL', '') or

'https://' in os.environ.get('SINGLESTOREDB_URL', ''),

any(x in os.environ.get('SINGLESTOREDB_URL', '') for x in ['http://', 'https://', '+http://', '+https://']),

Copilot · 2025-11-14T19:18:03Z

singlestoredb/tests/conftest.py

+from collections.abc import Iterator
+from typing import Optional
+
+import pytest


Module 'pytest' is imported with both 'import' and 'import from'.
Module 'singlestoredb.pytest' is imported with both 'import' and 'import from'.

Copilot · 2025-11-14T19:18:03Z

singlestoredb/tests/conftest.py

+
+from singlestoredb.pytest import _TestContainerManager
+from singlestoredb.pytest import execution_mode  # noqa: F401
+from singlestoredb.pytest import ExecutionMode  # noqa: F401


Import of 'ExecutionMode' is not used.

Suggested change

from singlestoredb.pytest import ExecutionMode # noqa: F401

This commit enables running tests via HTTP Data API when USE_DATA_API=1 is set, while setup operations still use MySQL protocol for commands that aren't supported over HTTP. Changes: - singlestoredb/pytest.py: Added http_connection_url property, fixed port mapping (9000 for Data API, 8080 for Studio), suppressed Docker container ID output - singlestoredb/tests/conftest.py: Added USE_DATA_API environment variable support, sets SINGLESTOREDB_INIT_DB_URL for setup operations, added clear logging for Docker lifecycle and connection mode - singlestoredb/tests/utils.py: Modified load_sql to always use SINGLESTOREDB_INIT_DB_URL when set for setup operations like SET GLOBAL - singlestoredb/tests/test_plugin.py: Added skip marker for HTTP Data API mode since USE database doesn't work over HTTP - pyproject.toml: Added docker to test dependencies 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

kesmit13 temporarily deployed to Base November 14, 2025 19:12 — with GitHub Actions Inactive

kesmit13 requested a review from Copilot November 14, 2025 19:12

Copilot started reviewing on behalf of kesmit13 November 14, 2025 19:12 View session

Copilot finished reviewing on behalf of kesmit13 November 14, 2025 19:17

Copilot AI reviewed Nov 14, 2025

View reviewed changes

kesmit13 temporarily deployed to Base November 14, 2025 19:28 — with GitHub Actions Inactive

kesmit13 and others added 2 commits November 14, 2025 13:35

Minor fixes based on copilot review

28c0655

kesmit13 force-pushed the docker-all-tests branch from 82da2e4 to 28c0655 Compare November 14, 2025 19:36

kesmit13 had a problem deploying to Base November 14, 2025 19:36 — with GitHub Actions Failure

Update HTTP protocol check

cd49eed

kesmit13 temporarily deployed to Base November 14, 2025 19:45 — with GitHub Actions Inactive

kesmit13 had a problem deploying to Base November 14, 2025 19:51 — with GitHub Actions Failure

kesmit13 temporarily deployed to Base November 14, 2025 21:01 — with GitHub Actions Inactive

Minor doc changes

f6803d5

kesmit13 temporarily deployed to Base November 17, 2025 14:51 — with GitHub Actions Inactive

kesmit13 merged commit ea9af55 into main Nov 17, 2025
11 checks passed

kesmit13 deleted the docker-all-tests branch November 17, 2025 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Automatically invoke Docker when running pytest #106

Automatically invoke Docker when running pytest #106

Uh oh!

kesmit13 commented Nov 14, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Copilot AI Nov 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	raise RuntimeError('Failed to stop container.') from e
	raise RuntimeError('Failed to remove container.') from e

		'http://' in os.environ.get('SINGLESTOREDB_URL', '') or
		'https://' in os.environ.get('SINGLESTOREDB_URL', ''),

	'http://' in os.environ.get('SINGLESTOREDB_URL', '') or
	'https://' in os.environ.get('SINGLESTOREDB_URL', ''),
	any(x in os.environ.get('SINGLESTOREDB_URL', '') for x in ['http://', 'https://', '+http://', '+https://']),

Automatically invoke Docker when running pytest #106

Automatically invoke Docker when running pytest #106

Uh oh!

Conversation

kesmit13 commented Nov 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants