-
Notifications
You must be signed in to change notification settings - Fork 30
tests: Add parametrized validation test for manifest-only connectors #706
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
9465284
473f237
617c64f
ec5a6b1
8a3a248
104aac2
dc2416e
fcaf435
6aeb97b
5841083
79e9118
9282e0b
0a4b66e
a87f360
7f92efa
0ecc73d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,226 @@ | ||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
Unit tests for validating manifest.yaml files from the connector registry against the CDK schema. | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
This test suite fetches all manifest-only connectors from the Airbyte connector registry, | ||||||||||||||||||||||||||||||
downloads their manifest.yaml files from public endpoints, and validates them against | ||||||||||||||||||||||||||||||
the current declarative component schema defined in the CDK. | ||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import json | ||||||||||||||||||||||||||||||
import logging | ||||||||||||||||||||||||||||||
from pathlib import Path | ||||||||||||||||||||||||||||||
from typing import Any, Dict, List, Tuple | ||||||||||||||||||||||||||||||
from unittest.mock import patch | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import pytest | ||||||||||||||||||||||||||||||
import requests | ||||||||||||||||||||||||||||||
import yaml | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
from airbyte_cdk.sources.declarative.validators.validate_adheres_to_schema import ( | ||||||||||||||||||||||||||||||
ValidateAdheresToSchema, | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
logger = logging.getLogger(__name__) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
aaronsteers marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
EXCLUDED_CONNECTORS = [ | ||||||||||||||||||||||||||||||
] | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
CONNECTOR_REGISTRY_URL = "https://connectors.airbyte.com/files/registries/v0/oss_registry.json" | ||||||||||||||||||||||||||||||
MANIFEST_URL_TEMPLATE = "https://connectors.airbyte.com/files/metadata/airbyte/{connector_name}/latest/manifest.yaml" | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
VALIDATION_SUCCESSES = [] | ||||||||||||||||||||||||||||||
VALIDATION_FAILURES = [] | ||||||||||||||||||||||||||||||
DOWNLOAD_FAILURES = [] | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Global state variables (VALIDATION_SUCCESSES, VALIDATION_FAILURES, DOWNLOAD_FAILURES) are not thread-safe and could cause issues in parallel test execution. Consider using pytest fixtures or moving these to a class-based test structure.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||||||||||
def load_declarative_component_schema() -> Dict[str, Any]: | ||||||||||||||||||||||||||||||
"""Load the declarative component schema from the CDK.""" | ||||||||||||||||||||||||||||||
schema_path = ( | ||||||||||||||||||||||||||||||
Path(__file__).resolve().parent.parent.parent.parent | ||||||||||||||||||||||||||||||
/ "airbyte_cdk/sources/declarative/declarative_component_schema.yaml" | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
with open(schema_path, "r") as file: | ||||||||||||||||||||||||||||||
return yaml.safe_load(file) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
def get_manifest_only_connectors() -> List[Tuple[str, str]]: | ||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
Fetch manifest-only connectors from the registry. | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
Returns: | ||||||||||||||||||||||||||||||
List of tuples (connector_name, cdk_version) where cdk_version will be | ||||||||||||||||||||||||||||||
determined from the manifest.yaml file itself. | ||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||
response = requests.get(CONNECTOR_REGISTRY_URL, timeout=30) | ||||||||||||||||||||||||||||||
response.raise_for_status() | ||||||||||||||||||||||||||||||
registry = response.json() | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
manifest_connectors = [] | ||||||||||||||||||||||||||||||
for source in registry.get("sources", []): | ||||||||||||||||||||||||||||||
if source.get("language") == "manifest-only": | ||||||||||||||||||||||||||||||
connector_name = source.get("dockerRepository", "").replace("airbyte/", "") | ||||||||||||||||||||||||||||||
if connector_name: | ||||||||||||||||||||||||||||||
manifest_connectors.append((connector_name, None)) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
return manifest_connectors | ||||||||||||||||||||||||||||||
except Exception as e: | ||||||||||||||||||||||||||||||
pytest.fail(f"Failed to fetch connector registry: {e}") | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Type annotation and error handling improvements A few suggestions for this function:
Example with retries: from typing import Optional
import time
def get_manifest_only_connectors() -> List[Tuple[str, Optional[str]]]:
"""Fetch manifest-only connectors from the registry."""
max_retries = 3
for attempt in range(max_retries):
try:
response = requests.get(CONNECTOR_REGISTRY_URL, timeout=30)
response.raise_for_status()
# ... rest of the logic
except Exception as e:
if attempt == max_retries - 1:
raise RuntimeError(f"Failed to fetch connector registry after {max_retries} attempts: {e}")
time.sleep(2 ** attempt) # Exponential backoff wdyt? 🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
def download_manifest(connector_name: str) -> Tuple[str, str]: | ||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
Download manifest.yaml for a connector. | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
Returns: | ||||||||||||||||||||||||||||||
Tuple of (manifest_content, cdk_version) where cdk_version is extracted | ||||||||||||||||||||||||||||||
from the manifest's version field. | ||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
url = MANIFEST_URL_TEMPLATE.format(connector_name=connector_name) | ||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||
response = requests.get(url, timeout=30) | ||||||||||||||||||||||||||||||
response.raise_for_status() | ||||||||||||||||||||||||||||||
manifest_content = response.text | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
manifest_dict = yaml.safe_load(manifest_content) | ||||||||||||||||||||||||||||||
cdk_version = manifest_dict.get("version", "unknown") | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
return manifest_content, cdk_version | ||||||||||||||||||||||||||||||
except Exception as e: | ||||||||||||||||||||||||||||||
DOWNLOAD_FAILURES.append((connector_name, str(e))) | ||||||||||||||||||||||||||||||
raise | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
def get_manifest_only_connector_names() -> List[str]: | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function calls get_manifest_only_connectors() which makes a network request to the registry. When used in @pytest.mark.parametrize, this network call happens during test collection, potentially slowing down test discovery. Consider caching the result or using a different approach. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
Get all manifest-only connector names from the registry. | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
Returns: | ||||||||||||||||||||||||||||||
List of connector names (e.g., "source-hubspot") | ||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
connectors = get_manifest_only_connectors() | ||||||||||||||||||||||||||||||
return [connector_name for connector_name, _ in connectors] | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider adding CI-friendly connector limits The PR objectives mention "considering a CI-friendly limit on processed connectors" since there are ~475 connectors. Running all of them might cause CI timeouts or excessive resource usage. Would you consider adding an optional limit mechanism? For example: import os
def get_manifest_only_connector_names() -> List[str]:
"""Get manifest-only connector names with optional limit for CI."""
connectors = get_manifest_only_connectors()
connector_names = [name for name, _ in connectors]
# Allow limiting connectors for CI runs
max_connectors = os.getenv("MAX_CONNECTORS_TO_TEST")
if max_connectors:
limit = int(max_connectors)
if limit > 0:
connector_names = connector_names[:limit]
logger.info(f"Limited to testing {limit} connectors (CI mode)")
return connector_names This would allow CI to run with 🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
@pytest.mark.parametrize("connector_name", get_manifest_only_connector_names()) | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid network during test collection: move parametrization to pytest_generate_tests Calling get_manifest_only_connector_names() at import time makes test collection flaky/slow and can fail entirely if the registry is down. Shall we move parametrization to pytest_generate_tests and keep collection side-effect free, wdyt? Apply this diff to remove import-time network access: -@pytest.mark.parametrize("connector_name", get_manifest_only_connector_names())
+# Parametrization moved to pytest_generate_tests to avoid network at collection time. Add this hook at the end of the file to perform parametrization safely at runtime: def pytest_generate_tests(metafunc):
if "connector_name" in metafunc.fixturenames:
try:
names = get_manifest_only_connector_names()
except Exception as e:
names = []
tr = metafunc.config.pluginmanager.get_plugin("terminalreporter")
if tr:
tr.write_line(f"[manifest-validator] Skipping connector parametrization due to registry error: {e}")
metafunc.parametrize("connector_name", names) 🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||
def test_manifest_validates_against_schema(connector_name: str): | ||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
Test that manifest.yaml files from the registry validate against the CDK schema. | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
Args: | ||||||||||||||||||||||||||||||
connector_name: Name of the connector (e.g., "source-hubspot") | ||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
# Download manifest first to get CDK version | ||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||
manifest_content, cdk_version = download_manifest(connector_name) | ||||||||||||||||||||||||||||||
except Exception as e: | ||||||||||||||||||||||||||||||
pytest.fail(f"Failed to download manifest for {connector_name}: {e}") | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
if (connector_name, cdk_version) in EXCLUDED_CONNECTORS: | ||||||||||||||||||||||||||||||
pytest.skip( | ||||||||||||||||||||||||||||||
f"Skipping {connector_name} - connector declares it is compatible with " | ||||||||||||||||||||||||||||||
f"CDK version {cdk_version} but is known to fail validation" | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||
manifest_dict = yaml.safe_load(manifest_content) | ||||||||||||||||||||||||||||||
except yaml.YAMLError as e: | ||||||||||||||||||||||||||||||
error_msg = f"Invalid YAML in manifest for {connector_name}: {e}" | ||||||||||||||||||||||||||||||
VALIDATION_FAILURES.append((connector_name, cdk_version, error_msg)) | ||||||||||||||||||||||||||||||
pytest.fail(error_msg) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
schema = load_declarative_component_schema() | ||||||||||||||||||||||||||||||
validator = ValidateAdheresToSchema(schema=schema) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||
validator.validate(manifest_dict) | ||||||||||||||||||||||||||||||
|
validator.validate(manifest_dict) | |
# Use the module-level validator to validate the manifest | |
try: | |
VALIDATOR.validate(manifest_dict) |
Copilot uses AI. Check for mistakes.
Uh oh!
There was an error while loading. Please reload this page.