From 5c86745db95c64c5a34717e1600901742211e3b7 Mon Sep 17 00:00:00 2001 From: lrangine <19699092+lokeshrangineni@users.noreply.github.com> Date: Wed, 8 Jan 2025 09:09:11 -0500 Subject: [PATCH] * Separating the tests related to remote and rbac functionality. * Added a new test marker to separate the tests related to rbac and remote functionality. * Added a new github action to perform tests related to rbac and remote functionality. * Filtered the rbac integration tests in the current github job. and added new make target to run the new tests. Signed-off-by: lrangine <19699092+lokeshrangineni@users.noreply.github.com> --- .../operator-e2e-integration-tests.yml | 4 ++ .../pr_remote_rbac_integration_tests.yml | 58 +++++++++++++++++++ Makefile | 13 +++++ sdk/python/pytest.ini | 1 + sdk/python/tests/conftest.py | 4 ++ sdk/python/tests/integration/conftest.py | 21 ++++++- .../universal/data_source_creator.py | 9 +++ .../universal/data_sources/file.py | 14 +++++ .../online_store/test_remote_online_store.py | 1 + .../registration/test_universal_registry.py | 5 +- 10 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/pr_remote_rbac_integration_tests.yml diff --git a/.github/workflows/operator-e2e-integration-tests.yml b/.github/workflows/operator-e2e-integration-tests.yml index cbb505c3fe8..a06e793410e 100644 --- a/.github/workflows/operator-e2e-integration-tests.yml +++ b/.github/workflows/operator-e2e-integration-tests.yml @@ -10,6 +10,10 @@ on: - opened - synchronize - labeled + paths-ignore: + - 'community/**' + - 'docs/**' + - 'examples/**' jobs: operator-e2e-tests: diff --git a/.github/workflows/pr_remote_rbac_integration_tests.yml b/.github/workflows/pr_remote_rbac_integration_tests.yml new file mode 100644 index 00000000000..98fa5a52c58 --- /dev/null +++ b/.github/workflows/pr_remote_rbac_integration_tests.yml @@ -0,0 +1,58 @@ +name: pr-remote-rbac-integration-tests +# This runs the integration tests related to rbac functionality and remote registry and online features. + +on: + pull_request: + types: + - opened + - synchronize + - labeled + paths-ignore: + - 'community/**' + - 'docs/**' + - 'examples/**' + +jobs: + remote-rbac-integration-tests-python: + if: + ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || + (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && + github.event.pull_request.base.repo.full_name == 'feast-dev/feast' + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + python-version: [ "3.11" ] + os: [ ubuntu-latest ] + env: + OS: ${{ matrix.os }} + PYTHON: ${{ matrix.python-version }} + steps: + - uses: actions/checkout@v4 + with: + repository: ${{ github.event.repository.full_name }} # Uses the full repository name + ref: ${{ github.ref }} # Uses the ref from the event + token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token + submodules: recursive + - name: Setup Python + uses: actions/setup-python@v5 + id: setup-python + with: + python-version: ${{ matrix.python-version }} + architecture: x64 + - name: Install uv + run: curl -LsSf https://astral.sh/uv/install.sh | sh + - name: Get uv cache dir + id: uv-cache + run: | + echo "dir=$(uv cache dir)" >> $GITHUB_OUTPUT + - name: uv cache + uses: actions/cache@v4 + with: + path: ${{ steps.uv-cache.outputs.dir }} + key: ${{ runner.os }}-${{ matrix.python-version }}-uv-${{ hashFiles(format('**/py{0}-ci-requirements.txt', matrix.python-version)) }} + - name: Install dependencies + run: make install-python-dependencies-ci + - name: Test rbac and remote feature integration tests + if: ${{ always() }} # this will guarantee that step won't be canceled and resources won't leak + run: make test-python-integration-rbac-remote diff --git a/Makefile b/Makefile index 79628169218..0d08f002bb3 100644 --- a/Makefile +++ b/Makefile @@ -107,6 +107,8 @@ test-python-unit: test-python-integration: python -m pytest --tb=short -v -n 8 --integration --color=yes --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \ -k "(not snowflake or not test_historical_features_main)" \ + -m "not rbac_remote_integration_test" \ + --log-cli-level=INFO -s \ sdk/python/tests test-python-integration-local: @@ -114,6 +116,17 @@ test-python-integration-local: FEAST_LOCAL_ONLINE_CONTAINER=True \ python -m pytest --tb=short -v -n 8 --color=yes --integration --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \ -k "not test_lambda_materialization and not test_snowflake_materialization" \ + -m "not rbac_remote_integration_test" \ + --log-cli-level=INFO -s \ + sdk/python/tests + +test-python-integration-rbac-remote: + FEAST_IS_LOCAL_TEST=True \ + FEAST_LOCAL_ONLINE_CONTAINER=True \ + python -m pytest --tb=short -v -n 8 --color=yes --integration --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \ + -k "not test_lambda_materialization and not test_snowflake_materialization" \ + -m "rbac_remote_integration_test" \ + --log-cli-level=INFO -s \ sdk/python/tests test-python-integration-container: diff --git a/sdk/python/pytest.ini b/sdk/python/pytest.ini index a0736767601..d79459c0d0e 100644 --- a/sdk/python/pytest.ini +++ b/sdk/python/pytest.ini @@ -4,6 +4,7 @@ asyncio_mode = auto markers = universal_offline_stores: mark a test as using all offline stores. universal_online_stores: mark a test as using all online stores. + rbac_remote_integration_test: mark a integration test related to rbac and remote functionality. env = IS_TEST=True diff --git a/sdk/python/tests/conftest.py b/sdk/python/tests/conftest.py index 6e5f1e14870..c029648aeeb 100644 --- a/sdk/python/tests/conftest.py +++ b/sdk/python/tests/conftest.py @@ -310,6 +310,10 @@ def pytest_generate_tests(metafunc: pytest.Metafunc): pytest.mark.xdist_group(name=m) for m in c.offline_store_creator.xdist_groups() ] + # Check if there are any test markers associated with the creator and add them. + if c.offline_store_creator.test_markers(): + marks.extend(c.offline_store_creator.test_markers()) + _config_cache[c] = pytest.param(c, marks=marks) configs.append(_config_cache[c]) diff --git a/sdk/python/tests/integration/conftest.py b/sdk/python/tests/integration/conftest.py index 82f80b89927..21c9051d0d7 100644 --- a/sdk/python/tests/integration/conftest.py +++ b/sdk/python/tests/integration/conftest.py @@ -1,4 +1,7 @@ import logging +import random +import time +from multiprocessing import Manager import pytest from testcontainers.keycloak import KeycloakContainer @@ -9,14 +12,30 @@ from tests.utils.auth_permissions_util import setup_permissions_on_keycloak logger = logging.getLogger(__name__) +logger.setLevel(logging.INFO) + +shared_state = Manager().dict() @pytest.fixture(scope="session") def start_keycloak_server(): + # Add random sleep between 0 and 2 before checking the state to avoid concurrency issues. + random_sleep_time = random.uniform(0, 2) + time.sleep(random_sleep_time) + + # If the Keycloak instance is already started (in any worker), reuse it + if shared_state.get("keycloak_started", False): + return shared_state["keycloak_url"] logger.info("Starting keycloak instance") with KeycloakContainer("quay.io/keycloak/keycloak:24.0.1") as keycloak_container: setup_permissions_on_keycloak(keycloak_container.get_client()) - yield keycloak_container.get_url() + shared_state["keycloak_started"] = True + shared_state["keycloak_url"] = keycloak_container.get_url() + yield shared_state["keycloak_url"] + + # After the fixture is done, cleanup the shared state + del shared_state["keycloak_started"] + del shared_state["keycloak_url"] @pytest.fixture(scope="session") diff --git a/sdk/python/tests/integration/feature_repos/universal/data_source_creator.py b/sdk/python/tests/integration/feature_repos/universal/data_source_creator.py index 513a94ee210..467db4dddce 100644 --- a/sdk/python/tests/integration/feature_repos/universal/data_source_creator.py +++ b/sdk/python/tests/integration/feature_repos/universal/data_source_creator.py @@ -2,6 +2,7 @@ from typing import Dict, Optional import pandas as pd +from _pytest.mark import MarkDecorator from feast.data_source import DataSource from feast.feature_logging import LoggingDestination @@ -64,3 +65,11 @@ def teardown(self): @staticmethod def xdist_groups() -> list[str]: return [] + + @staticmethod + def test_markers() -> list[MarkDecorator]: + """ + return the array of test markers to add dynamically to the tests created by this creator method. override this method in your implementations. By default, it will not add any markers. + :return: + """ + return [] diff --git a/sdk/python/tests/integration/feature_repos/universal/data_sources/file.py b/sdk/python/tests/integration/feature_repos/universal/data_sources/file.py index 1d33402e012..6f6e5d68133 100644 --- a/sdk/python/tests/integration/feature_repos/universal/data_sources/file.py +++ b/sdk/python/tests/integration/feature_repos/universal/data_sources/file.py @@ -11,7 +11,9 @@ import pandas as pd import pyarrow as pa import pyarrow.parquet as pq +import pytest import yaml +from _pytest.mark import MarkDecorator from minio import Minio from testcontainers.core.generic import DockerContainer from testcontainers.core.waiting_utils import wait_for_logs @@ -372,6 +374,10 @@ def __init__(self, project_name: str, *args, **kwargs): self.server_port: int = 0 self.proc: Optional[Popen[bytes]] = None + @staticmethod + def test_markers() -> list[MarkDecorator]: + return [pytest.mark.rbac_remote_integration_test] + def setup(self, registry: RegistryConfig): parent_offline_config = super().create_offline_store_config() config = RepoConfig( @@ -418,6 +424,10 @@ def __init__(self, project_name: str, *args, **kwargs): self.server_port: int = 0 self.proc: Optional[Popen[bytes]] = None + @staticmethod + def test_markers() -> list[MarkDecorator]: + return [pytest.mark.rbac_remote_integration_test] + def setup(self, registry: RegistryConfig): parent_offline_config = super().create_offline_store_config() config = RepoConfig( @@ -515,6 +525,10 @@ def __init__(self, project_name: str, *args, **kwargs): def xdist_groups() -> list[str]: return ["keycloak"] + @staticmethod + def test_markers() -> list[MarkDecorator]: + return [pytest.mark.rbac_remote_integration_test] + def setup(self, registry: RegistryConfig): parent_offline_config = super().create_offline_store_config() config = RepoConfig( diff --git a/sdk/python/tests/integration/online_store/test_remote_online_store.py b/sdk/python/tests/integration/online_store/test_remote_online_store.py index 285253dfaaf..eb03fd0c3c5 100644 --- a/sdk/python/tests/integration/online_store/test_remote_online_store.py +++ b/sdk/python/tests/integration/online_store/test_remote_online_store.py @@ -22,6 +22,7 @@ @pytest.mark.integration +@pytest.mark.rbac_remote_integration_test @pytest.mark.parametrize( "tls_mode", [("True", "True"), ("True", "False"), ("False", "")], indirect=True ) diff --git a/sdk/python/tests/integration/registration/test_universal_registry.py b/sdk/python/tests/integration/registration/test_universal_registry.py index 5e06247ebbb..3819d168d78 100644 --- a/sdk/python/tests/integration/registration/test_universal_registry.py +++ b/sdk/python/tests/integration/registration/test_universal_registry.py @@ -344,7 +344,10 @@ def mock_remote_registry(): marks=pytest.mark.xdist_group(name="mysql_registry"), ), lazy_fixture("sqlite_registry"), - lazy_fixture("mock_remote_registry"), + pytest.param( + lazy_fixture("mock_remote_registry"), + marks=pytest.mark.rbac_remote_integration_test, + ), ] sql_fixtures = [