Unit tests parsing precedence instance (#26)

* BUGFIX: Corrected tag interpolation for unpack_tags

* Add unit tests for tag parsing, config precedence and instance creation validation

Author: ConnorCoull

src/ec2sandbox/_unpack_tags.py

@@ -12,7 +12,7 @@ def unpack_tags(tags: str | None) -> Tuple[Tuple[str, str], ...]:
         except ValueError:
             raise ValueError(
                 "Tags must be in the format 'key1=value1;key2=value2', "
-                "but instead got {tags}"
+                f"but instead got {tags}"
             )
     return tuple(tags_unpacked)
 

tests/ec2sandboxtest/test_create_instance.py

@@ -0,0 +1,82 @@
+from typing import Any
+from unittest import mock
+
+import pytest
+
+from ec2sandbox._instance_provider import DefaultEc2InstanceProvider
+from ec2sandbox.schema import Ec2SandboxEnvironmentConfig
+
+
+def _make_config(**overrides: Any) -> Ec2SandboxEnvironmentConfig:
+    defaults: dict[str, Any] = dict(
+        instance_type="t3a.micro",
+        ami_id="ami-123",
+        region="eu-west-2",
+        security_group_id="sg-1",
+        subnet_id="subnet-1",
+        instance_profile="profile-1",
+        s3_bucket="bucket-1",
+    )
+    defaults.update(overrides)
+    return Ec2SandboxEnvironmentConfig(**defaults)
+
+
+def _make_provider_with_mocks(
+    config: Ec2SandboxEnvironmentConfig,
+) -> tuple[DefaultEc2InstanceProvider, mock.MagicMock]:
+    ec2_client = mock.MagicMock()
+    ec2_client.run_instances.return_value = {"Instances": [{"InstanceId": "i-abc"}]}
+
+    ssm_client = mock.MagicMock()
+    ssm_client.describe_instance_information.return_value = {
+        "InstanceInformationList": [{"PingStatus": "Online"}]
+    }
+
+    def client(service: str, **kwargs: Any) -> mock.MagicMock:
+        if service == "ec2":
+            return ec2_client
+        if service == "ssm":
+            return ssm_client
+        raise AssertionError(f"unexpected client: {service}")
+
+    session = mock.MagicMock()
+    session.client.side_effect = client
+
+    return DefaultEc2InstanceProvider(config, session), ec2_client
+
+
+@pytest.mark.asyncio
+async def test_missing_config_fields_fail_fast() -> None:
+    """Missing config fields are all named, and no AWS client is built."""
+    config = _make_config(subnet_id=None, s3_bucket=None)
+    session = mock.MagicMock()
+    provider = DefaultEc2InstanceProvider(config, session)
+
+    with pytest.raises(ValueError) as excinfo:
+        await provider.create_instance(
+            instance_type="t3a.micro",
+            ami_id="ami-123",
+            tags=[("Name", "x")],
+        )
+
+    # Both missing fields are reported at once, not just the first found.
+    assert "subnet_id" in str(excinfo.value)
+    assert "s3_bucket" in str(excinfo.value)
+    # Fail-fast: validation must reject before any AWS interaction.
+    session.client.assert_not_called()
+
+
+@pytest.mark.asyncio
+async def test_tags_reach_run_instances() -> None:
+    """A tag passed to create_instance appears in the run_instances call."""
+    provider, ec2_client = _make_provider_with_mocks(_make_config())
+
+    await provider.create_instance(
+        instance_type="t3a.micro",
+        ami_id="ami-123",
+        tags=[("sentinel_key", "sentinel_value")],
+    )
+
+    tag_specs = ec2_client.run_instances.call_args.kwargs["TagSpecifications"]
+    sentinel = {"Key": "sentinel_key", "Value": "sentinel_value"}
+    assert sentinel in tag_specs[0]["Tags"]

tests/ec2sandboxtest/test_from_settings.py

@@ -0,0 +1,66 @@
+import os
+from pathlib import Path
+from unittest import mock
+
+import pytest
+
+from ec2sandbox.schema import Ec2SandboxEnvironmentConfig
+
+
+@pytest.fixture(autouse=True)
+def _run_outside_repo(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
+    """Keep a contributor's local .env file out of pydantic-settings' reach."""
+    monkeypatch.chdir(tmp_path)
+
+
+def env_vars_all() -> dict[str, str]:
+    # AMI_ID must always be present: from_settings resolves a missing AMI
+    # via a real SSM lookup, which these tests must never reach.
+    return {
+        "INSPECT_EC2_SANDBOX_REGION": "eu-west-2",
+        "INSPECT_EC2_SANDBOX_VPC_ID": "vpc-123",
+        "INSPECT_EC2_SANDBOX_SECURITY_GROUP_ID": "sg-456",
+        "INSPECT_EC2_SANDBOX_SUBNET_ID": "subnet-654321",
+        "INSPECT_EC2_SANDBOX_AMI_ID": "ami-789",
+        "INSPECT_EC2_SANDBOX_INSTANCE_PROFILE": "profile-1",
+        "INSPECT_EC2_SANDBOX_S3_BUCKET": "fake-bucket",
+    }
+
+
+def test_kwarg_overrides_env_var() -> None:
+    """Config kwargs take precedence over environment variables."""
+    env_vars = env_vars_all()
+    env_vars["INSPECT_EC2_SANDBOX_INSTANCE_TYPE"] = "t3a.small"
+    with mock.patch.dict(os.environ, env_vars, clear=True):
+        config = Ec2SandboxEnvironmentConfig.from_settings(instance_type="t3a.xlarge")
+    assert config.instance_type == "t3a.xlarge"
+
+
+def test_region_falls_back_to_aws_region() -> None:
+    """AWS_REGION is used when INSPECT_EC2_SANDBOX_REGION is unset."""
+    env_vars = env_vars_all()
+    env_vars.pop("INSPECT_EC2_SANDBOX_REGION")
+    env_vars["AWS_REGION"] = "eu-west-1"
+    with mock.patch.dict(os.environ, env_vars, clear=True):
+        config = Ec2SandboxEnvironmentConfig.from_settings()
+    assert config.region == "eu-west-1"
+
+
+def test_missing_region_raises_value_error() -> None:
+    """With no region from any source, from_settings raises a clear error."""
+    env_vars = env_vars_all()
+    env_vars.pop("INSPECT_EC2_SANDBOX_REGION")
+    with mock.patch.dict(os.environ, env_vars, clear=True):
+        with pytest.raises(ValueError, match="Region must be specified"):
+            Ec2SandboxEnvironmentConfig.from_settings()
+
+
+def test_s3_key_prefix_leading_slash_raises() -> None:
+    """A leading '/' in s3_key_prefix is rejected; normal values pass."""
+    with mock.patch.dict(os.environ, env_vars_all(), clear=True):
+        with pytest.raises(ValueError, match="must not start with"):
+            Ec2SandboxEnvironmentConfig.from_settings(s3_key_prefix="/bad")
+        config = Ec2SandboxEnvironmentConfig.from_settings(
+            s3_key_prefix="sandbox-comms"
+        )
+    assert config.s3_key_prefix == "sandbox-comms"

tests/ec2sandboxtest/test_unpack_tags.py

@@ -0,0 +1,54 @@
+import re
+
+import pytest
+
+from ec2sandbox._unpack_tags import convert_tags_for_aws_interface, unpack_tags
+
+
+def test_parses_multiple_tags() -> None:
+    """Pin the documented 'key1=value1;key2=value2' delimiter contract."""
+    assert unpack_tags("team=research;env=dev") == (
+        ("team", "research"),
+        ("env", "dev"),
+    )
+
+
+def test_missing_equals_raises_value_error() -> None:
+    """A segment with no '=' is rejected, not passed through mangled."""
+    with pytest.raises(ValueError):
+        unpack_tags("noequals")
+
+
+def test_equals_in_value_raises_value_error() -> None:
+    """A value containing '=' is rejected: parsing is a strict single split."""
+    # Pins current behaviour deliberately: switching to split-on-first-'='
+    # (allowing '=' in values) would be a behaviour change and should
+    # surface here.
+    with pytest.raises(ValueError):
+        unpack_tags("key=a=b")
+
+
+def test_empty_and_none_return_empty_tuple() -> None:
+    """No extra tags configured (the default case) yields an empty tuple."""
+    assert unpack_tags(None) == ()
+    assert unpack_tags("") == ()
+
+
+def test_error_message_contains_offending_input() -> None:
+    """The parse error names the input that failed to parse."""
+    with pytest.raises(ValueError, match=re.escape("bad-tag-input")):
+        unpack_tags("bad-tag-input")
+
+
+def test_convert_tags_shape_for_aws() -> None:
+    """Tags convert to the TagSpecifications shape cleanup relies on."""
+    result = convert_tags_for_aws_interface("instance", (("k1", "v1"), ("k2", "v2")))
+    assert result == [
+        {
+            "ResourceType": "instance",
+            "Tags": [
+                {"Key": "k1", "Value": "v1"},
+                {"Key": "k2", "Value": "v2"},
+            ],
+        }
+    ]