docs(skills): test-skill cleanups and gotchas surfaced from supadata PR (#40)

TheRealAgentK · web-flow · commit 9d1b48ad6b30 · 2026-05-04T15:08:15.000+12:00
* docs(skills): simplify test boilerplate and surface root conftest fixtures (writing-unit-tests) Test files in autohive-integrations don't need the heavy `importlib.util.spec_from_file_location` boilerplate when the integration source can be imported as a normal module. With a 2-line `sys.path.insert` in `tests/conftest.py`, plain `from <integration> import ...` works fine. The repo-wide `conftest.py` also already provides three fixtures every test should reuse instead of redeclaring locally: - `mock_context` — minimal ExecutionContext - `make_context` — factory for arbitrary auth shapes - `env_credentials` — `.env`-aware env-var lookup with skip support Changes: - Promote plain imports as the default boilerplate; keep the `importlib` form as a documented fallback for unusual layouts. - Replace the 'every file declares its own mock_context' guidance with the override-in-conftest pattern, so credential shapes are set once per integration and inherited by all tests. - Add a parametrize example to 'Testing Helper Functions' for collapsing tabular helper tests (e.g. ms_to_timestamp boundaries). - New 'Common Gotchas' entry: PyPI package name collision. When the integration folder name matches a PyPI package the source imports (e.g. supadata, dropbox), the empty `__init__.py` shadows the real package. The fix is to drop `__init__.py` — the validator treats it as optional, the Lambda runtime is unaffected, and tests no longer need `site.getsitepackages()` / `importlib` shims. Surfaced while reviewing Autohive-AI/autohive-integrations#280 (Supadata SDK 2.0 upgrade). * docs(skills): simplify boilerplate, prefer env_credentials, and add SDK-call variant (writing-integration-tests) Three updates aligned with the writing-unit-tests skill changes: 1. Replace the heavy `importlib` boilerplate with plain imports (matching the unit-tests skill). Cross-reference the unit-tests skill for the `importlib` fallback rather than duplicating it. 2. Promote `env_credentials` from the repo-wide `conftest.py` as the recommended way to handle API-key / token env vars. The fixture auto-loads `.env` and integrates with `pytest.skip` when a key is missing. Module-level `os.environ.get(...)` is still useful for object IDs (which env_credentials doesn't model directly), so the require_* helpers stay — but credentials should be read via env_credentials. 3. Add Variant 4: 'External Python SDK (no `context.fetch`)'. Some integrations (e.g. supadata) call a third-party Python SDK directly rather than going through `context.fetch`. For these, the aiohttp-wrapping fixture is irrelevant — the SDK does its own networking. The new variant is a 4-line fixture that just injects credentials via `make_context(auth=...)`. Updated the 'How to choose' guidance to a decision table covering all four variants by auth shape and networking pattern. Surfaced while reviewing Autohive-AI/autohive-integrations#280. * docs(skills): add auth-default and PyPI-collision gotchas (upgrading-sdk-v2) Two new entries in the 'Common Gotchas' section, both surfaced while reviewing Autohive-AI/autohive-integrations#280 (Supadata 1.0.0 → 2.0.0 upgrade) and the underlying production crash report in Autohive-AI/autohive-integrations#316: 1. Audit auth lookup defaults during the upgrade. Several 1.0.x integrations shipped with the wrong default *type* in their auth lookup, e.g.: context.auth.get('credentials', {}).get('api_key', {}) The `{}` default returns a dict when the field is empty, which crashes the upstream SDK that expects a string with TypeError — surfacing as a Lambda 500 / Raygun crash instead of the user-facing auth error it actually is. The 2.0.0 upgrade is the right moment to fix this because the auth path is being touched anyway to convert error returns to ActionError. 2. PyPI package name collision. When the integration folder is named after a PyPI package the source imports (e.g. `from supadata import Supadata` inside `supadata/`), the empty `__init__.py` shadows the real PyPI package and every test fails with ImportError. Drop the `__init__.py` — the validator treats it as optional, the Lambda runtime is unaffected, and tests can use plain imports. Cross-references the writing-unit-tests skill where the matching test-side gotcha now lives. * docs(skills): add missing os import in module-level constants snippet The new file-header boilerplate dropped `import os` (it's no longer needed once the importlib path-juggling is gone), but the 'Module-level constants' snippet a few sections later still uses `os.environ.get(...)`. Readers who copy both sections verbatim would hit `NameError` at import time. Add `import os` directly to the constants snippet and call out in prose that it should join the file-header imports. Keeps each snippet self-contained and executable. Caught by the codex review bot on PR #40.
diff --git a/skills/upgrading-sdk-v2/SKILL.md b/skills/upgrading-sdk-v2/SKILL.md
@@ -285,3 +285,17 @@ Before considering an integration upgraded, verify:
 7. **CI fetch pattern linter false positives**: The linter flags any variable named `response` accessed with `.get()` or `["..."]`. If a helper already unwraps `.data` and returns a plain dict, rename the variable in callers to avoid the match (e.g. `gql_result`, `body`, `api_data`).
 
 8. **Ruff config mismatch**: CI uses `../autohive-integrations-tooling/ruff.toml` with `line-length = 120`. Always pass `--config` when formatting or local results will differ from CI.
+
+9. **Audit auth lookup defaults during the upgrade**: 1.0.x integrations sometimes shipped with the wrong default type in their auth lookup, e.g. `context.auth.get("credentials", {}).get("api_key", {})`. The default `{}` returns a dict when the field is unset, which then crashes the upstream API client that expects a string — surfacing as a Lambda 500 / Raygun crash rather than the user-facing auth error it actually is. Fix to a string default that matches the field type:
+
+    ```python
+    # Before — returns {} on missing field, crashes upstream SDK with TypeError
+    api_key = context.auth.get("credentials", {}).get("api_key", {})
+
+    # After — returns "" on missing field; upstream auth error becomes ActionError cleanly
+    api_key = context.auth.get("credentials", {}).get("api_key", "")
+    ```
+
+    The 2.0.0 upgrade is the right time to catch this because you're already touching the auth path to convert error returns to `ActionError`.
+
+10. **PyPI package name collision**: If your integration folder name matches a PyPI package the source imports (e.g. an integration in `supadata/` that does `from supadata import Supadata`), an empty `<integration>/__init__.py` will shadow the real PyPI package and every test fails with `ImportError`. Drop `<integration>/__init__.py` — the validator's "missing __init__.py" warning is correct to ignore in this case, and the Lambda runtime is unaffected (the entry point is the action source file, not the package). See the `writing-unit-tests` skill for the matching test-side guidance.
diff --git a/skills/writing-integration-tests/SKILL.md b/skills/writing-integration-tests/SKILL.md
@@ -49,7 +49,7 @@ This double exclusion ensures integration tests never run accidentally.
 
 ### File Header (boilerplate)
 
-Every integration test file must start with this exact boilerplate. Replace `myintegration` with the actual integration name:
+With `sys.path` set up in `tests/conftest.py` (see the unit tests skill for the standard shape), an integration test file can use plain imports:
 
 ```python
 """
@@ -65,42 +65,49 @@ Never runs in CI — the default pytest marker filter (-m unit) excludes these,
 and the file naming (test_*_integration.py) is not matched by python_files.
 """
 
-import os
-import sys
-import importlib
+import pytest
+from unittest.mock import AsyncMock, MagicMock
+from autohive_integrations_sdk import FetchResponse
+from autohive_integrations_sdk.integration import ResultType
+
+from myintegration import myintegration
 
-_parent = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
-_deps = os.path.abspath(os.path.join(os.path.dirname(__file__), "../dependencies"))
-sys.path.insert(0, _parent)
-sys.path.insert(0, _deps)
+pytestmark = pytest.mark.integration
+```
 
-import pytest  # noqa: E402
-from unittest.mock import MagicMock, AsyncMock  # noqa: E402
-from autohive_integrations_sdk import FetchResponse  # noqa: E402
+Use the `importlib` fallback boilerplate from the unit tests skill only when plain imports won't work for the integration's layout.
 
-_spec = importlib.util.spec_from_file_location("myintegration_mod", os.path.join(_parent, "myintegration.py"))
-_mod = importlib.util.module_from_spec(_spec)
-_spec.loader.exec_module(_mod)
+## Environment Variables
 
-myintegration = _mod.myintegration  # the Integration instance
+### env_credentials Fixture (preferred)
 
-pytestmark = pytest.mark.integration
+The repo-wide [`conftest.py`](https://github.com/Autohive-AI/autohive-integrations/blob/master/conftest.py) provides an `env_credentials` fixture that auto-loads the project `.env` and reads variables on demand:
+
+```python
+@pytest.fixture
+def live_context(env_credentials, make_context):
+    api_key = env_credentials("MYINTEGRATION_API_KEY")
+    if not api_key:
+        pytest.skip("MYINTEGRATION_API_KEY not set — skipping integration tests")
+    return make_context(auth={"credentials": {"api_key": api_key}})
 ```
 
-## Environment Variables
+This is the recommended pattern for any test that needs API credentials: it skips automatically when the env var is missing, integrates with the project `.env`, and avoids per-test boilerplate.
 
-### Token and ID Setup
+### Module-level constants (when you need them everywhere)
 
-Define environment variables at module level. Use `os.environ.get` with an empty string default:
+If multiple fixtures or helpers need the same env var, define them at module level — but still drive skip behaviour off `env_credentials` inside the fixture, not at import time. Add `import os` to the file header alongside the other top-level imports:
 
 ```python
-ACCESS_TOKEN = os.environ.get("MYINTEGRATION_ACCESS_TOKEN", "")
+import os
+
 TEST_ITEM_ID = os.environ.get("MYINTEGRATION_TEST_ITEM_ID", "")
+TEST_PROJECT_ID = os.environ.get("MYINTEGRATION_TEST_PROJECT_ID", "")
 ```
 
 ### require_* Skip Helpers
 
-For tests that need specific object IDs, create `require_*` helpers that skip gracefully:
+For tests that need specific object IDs (which `env_credentials` doesn't model directly), create `require_*` helpers that skip gracefully:
 
 ```python
 def require_item_id():
@@ -215,7 +222,29 @@ def live_context():
     return ctx
 ```
 
-**How to choose**: Check the integration's `config.json` — if `auth.type` is `"platform"`, use Variant 3. If the action handler reads an API key from `context.auth` or env vars and sets headers manually, use Variant 2. If no auth is needed, use Variant 1.
+### Variant 4: External Python SDK (no `context.fetch`)
+
+Some integrations don't go through `context.fetch` at all — instead they instantiate a third-party Python SDK and let it make the HTTP calls (e.g. `from supadata import Supadata`). For these, the `aiohttp` wrapper is irrelevant: the SDK does its own networking. Just inject the credentials via `make_context` and the upstream library handles the rest:
+
+```python
+@pytest.fixture
+def live_context(env_credentials, make_context):
+    api_key = env_credentials("MYINTEGRATION_API_KEY")
+    if not api_key:
+        pytest.skip("MYINTEGRATION_API_KEY not set — skipping integration tests")
+    return make_context(auth={"credentials": {"api_key": api_key}})
+```
+
+This variant is the simplest of the four — no `real_fetch` definition needed. Use it whenever the integration's source imports a vendor SDK and calls it directly rather than calling `context.fetch`.
+
+**How to choose**:
+
+| Auth shape | Networking | Variant |
+|---|---|---|
+| None (public API) | `context.fetch` | 1 — No auth |
+| API key in `context.auth` or env | `context.fetch` | 2 — API key |
+| Platform OAuth (`config.auth.type == "platform"`) | `context.fetch` | 3 — Platform OAuth |
+| Any | External Python SDK call | 4 — External SDK |
 
 ## The Destructive Marker
 
diff --git a/skills/writing-unit-tests/SKILL.md b/skills/writing-unit-tests/SKILL.md
@@ -42,21 +42,55 @@ hubspot/tests/
 
 ### conftest.py
 
-Every `tests/` directory should include a `conftest.py` with this standard content:
+Every `tests/` directory should include a `conftest.py`. At minimum it puts the integration source on `sys.path` so test files can use plain imports:
 
 ```python
-import sys
 import os
+import sys
+
+# Make <integration>.py importable as a top-level module.
+sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "..")))
+```
+
+This lets test files do `from myintegration import myintegration` (or `from myintegration_module import ...`) without per-file boilerplate.
 
-# Allow 'from context import ...' to work when pytest runs from repo root
-sys.path.insert(0, os.path.dirname(__file__))
+If the integration reads credentials from `context.auth`, also override the repo-wide `mock_context` fixture here so every test in this directory inherits credentials of the right shape:
+
+```python
+from unittest.mock import AsyncMock, MagicMock
+import pytest
+
+@pytest.fixture
+def mock_context():
+    """Mock ExecutionContext pre-loaded with this integration's credentials."""
+    ctx = MagicMock(name="ExecutionContext")
+    ctx.fetch = AsyncMock(name="fetch")
+    ctx.auth = {"credentials": {"api_key": "test_api_key"}}  # nosec B105
+    return ctx
 ```
 
 The `_unit.py` suffix is required — CI uses it to discover unit tests.
 
 ### File Header (boilerplate)
 
-Every test file must start with this exact boilerplate. Replace `myintegration` with the actual integration name and `myintegration.py` with the actual entry point file:
+Every test file starts with the same shape: imports, an `pytestmark = pytest.mark.unit` line, and the integration imports. With `sys.path` set up in `tests/conftest.py` (above), test files can use plain imports:
+
+```python
+import pytest
+from unittest.mock import AsyncMock, MagicMock, patch
+from autohive_integrations_sdk import FetchResponse
+from autohive_integrations_sdk.integration import ResultType
+
+from myintegration import myintegration
+# Also import any helpers you want to test directly:
+# from myintegration import parse_response, my_helper
+
+pytestmark = pytest.mark.unit
+```
+
+#### Fallback boilerplate (only when plain imports won't work)
+
+If the integration source can't be imported as a normal module — for example when the file lives at an unusual path or has been renamed away from the integration's folder name — use the explicit `importlib` loader:
 
 ```python
 import os
@@ -77,31 +111,31 @@ _spec = importlib.util.spec_from_file_location("myintegration_mod", os.path.join
 _mod = importlib.util.module_from_spec(_spec)
 _spec.loader.exec_module(_mod)
 
-myintegration = _mod.myintegration  # the Integration instance
-# Also import any helper functions you need to test directly:
-# parse_response = _mod.parse_response
-# my_helper = _mod.my_helper
+myintegration = _mod.myintegration
 
 pytestmark = pytest.mark.unit
 ```
 
-Add `from unittest.mock import patch` if you need to patch `asyncio.sleep` or environment variables.
+Prefer plain imports — reach for the `importlib` form only when there's a concrete reason it's needed.
 
 ### mock_context Fixture
 
-Every test file needs this fixture:
+The repo-wide [`conftest.py`](https://github.com/Autohive-AI/autohive-integrations/blob/master/conftest.py) already provides three fixtures every test can use:
+
+- `mock_context` — minimal `ExecutionContext` with `ctx.auth = {}` and `ctx.fetch` as an `AsyncMock`
+- `make_context` — factory for building a context with arbitrary `auth=...`
+- `env_credentials` — helper that reads env vars (with `.env` autoloaded), returns `None` if missing
+
+You don't need to redeclare these. Override `mock_context` only when the integration reads credentials from `context.auth` and you want every test to inherit the shape — see the snippet in the conftest.py section above.
+
+For one-off credential shapes inside a single test, use `make_context`:
 
 ```python
-@pytest.fixture
-def mock_context():
-    ctx = MagicMock(name="ExecutionContext")
-    ctx.fetch = AsyncMock(name="fetch")
-    ctx.auth = {}
-    return ctx
+async def test_with_custom_auth(make_context):
+    ctx = make_context(auth={"credentials": {"api_key": "different_key"}})  # nosec B105
+    ...
 ```
 
-If the integration reads credentials from `context.auth`, populate it to match the auth shape in `config.json`.
-
 **Platform OAuth** (`config.auth.type == "platform"`): The SDK wraps OAuth tokens in a standard envelope:
 
 ```python
@@ -361,6 +395,23 @@ class TestParseResponse:
         assert result == {"key": "value"}
 ```
 
+For helpers with several near-identical input/output cases, prefer `@pytest.mark.parametrize` over a separate test method per case — same coverage, easier to extend with new boundary rows:
+
+```python
+class TestMsToTimestamp:
+    @pytest.mark.parametrize(
+        "milliseconds, expected",
+        [
+            (0, "00:00:00,000"),
+            (1000, "00:00:01,000"),
+            (60_000, "00:01:00,000"),
+            (3_600_000, "01:00:00,000"),
+        ],
+    )
+    def test_ms_to_timestamp(self, milliseconds: int, expected: str):
+        assert ms_to_timestamp(milliseconds) == expected
+```
+
 ## Test Organization
 
 ### One class per action
@@ -448,6 +499,8 @@ Every action should have at minimum:
 
 7. **Unused variables**: If you call `execute_action` only to verify `mock_context.fetch.call_args`, don't assign the result to a variable — ruff will flag it as unused. Use `await integration.execute_action(...)` without assignment.
 
+8. **PyPI package name collision**: If your integration folder is named after a PyPI package the source imports (e.g. an integration in `supadata/` that does `from supadata import Supadata`), an empty `<integration>/__init__.py` will shadow the real PyPI package — every test fails with `ImportError`. The fix is to **delete `<integration>/__init__.py`**: the validator treats it as optional, the Lambda runtime is unaffected, and `from <package> import ...` then resolves cleanly to site-packages. Don't paper over the shadow with `site.getsitepackages()` / `importlib` shims in the test files.
+
 ## Reference Implementations
 
 Look at these integrations for well-tested examples:
