Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Separating the RBAC and Remote related integration tests. #4905

Merged
merged 1 commit into from
Jan 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/operator-e2e-integration-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ on:
- opened
- synchronize
- labeled
paths-ignore:
- 'community/**'
- 'docs/**'
- 'examples/**'

jobs:
operator-e2e-tests:
Expand Down
58 changes: 58 additions & 0 deletions .github/workflows/pr_remote_rbac_integration_tests.yml
Original file line number Diff line number Diff line change
@@ -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" ]

Choose a reason for hiding this comment

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

only 3.11?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

current integration tests are only running on 3.11. so kept the same for this action also. Should we add another python version for all integration tests?

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
13 changes: 13 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,26 @@ 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:
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 "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:
Expand Down
1 change: 1 addition & 0 deletions sdk/python/pytest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions sdk/python/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down
21 changes: 20 additions & 1 deletion sdk/python/tests/integration/conftest.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import logging
import random
import time
from multiprocessing import Manager

import pytest
from testcontainers.keycloak import KeycloakContainer
Expand All @@ -9,14 +12,30 @@
from tests.utils.auth_permissions_util import setup_permissions_on_keycloak

logger = logging.getLogger(__name__)
logger.setLevel(logging.INFO)
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to set log level as INFO as default is warning https://github.com/feast-dev/feast/blob/master/sdk/python/feast/cli.py#L84

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This log is only for integration tests to display info logs. Keeping it info helped to display the important logs during the test runs. These logs purged after the new run per PR only. It does not harm to keep it Info but also if you want we can change it WARN as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can keep Feast default log level, until unless it's required to see the info message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can update in the next PR as this is merged.


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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 []
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down
Loading