refactor: contract fixtures and simplification

Author: Daanvdplas

tests/common.rs

@@ -1,40 +1,48 @@
-use crate::common::{is_error, is_success, text, Contract, TestContext};
+use crate::common::{is_error, is_success, text, TestEnv};
 use anyhow::Result;
 use pop_mcp_server::tools::build::contract::{build_contract, BuildContractParams};
+use pop_mcp_server::tools::new::contract::{create_contract, CreateContractParams};
 
 #[test]
 fn build_contract_nonexistent_path_fails() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
+    let env = TestEnv::new()?;
     let params = BuildContractParams {
         path: "/nonexistent/path/to/contract".to_string(),
         release: None,
     };
 
-    let result = build_contract(&executor, params)?;
+    let result = build_contract(env.executor(), params)?;
     assert!(is_error(&result));
     assert!(text(&result)?.contains("Build failed"));
     Ok(())
 }
 
 #[test]
 fn build_contract_creates_ink_artifacts() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
-    let contract = Contract::with_context(ctx, &executor, "build_test")?;
+    let env = TestEnv::new()?;
 
-    let build_params = BuildContractParams {
-        path: contract.path.to_string_lossy().to_string(),
-        release: None,
-    };
+    // Create a fresh contract
+    create_contract(
+        env.executor(),
+        CreateContractParams {
+            name: "build_test".to_string(),
+            template: "standard".to_string(),
+        },
+    )?;
 
-    let result = build_contract(&executor, build_params)?;
-    assert!(is_success(&result));
+    let contract_path = env.workdir().join("build_test");
 
-    let output = text(&result)?;
-    assert!(output.contains("Build successful"));
+    // Build it
+    let result = build_contract(
+        env.executor(),
+        BuildContractParams {
+            path: contract_path.display().to_string(),
+            release: None,
+        },
+    )?;
 
-    // Verify build artifacts exist
-    assert!(contract.path.join("target/ink").exists());
+    assert!(is_success(&result));
+    assert!(text(&result)?.contains("Build successful"));
+    assert!(contract_path.join("target/ink").exists());
     Ok(())
 }

tests/tools/build/contract.rs

@@ -1,13 +1,10 @@
-use crate::common::{is_error, is_success, text, Contract, TestContext, DEFAULT_SURI};
-use anyhow::{anyhow, Result};
+use crate::common::{is_error, is_success, text, Contract, InkNode, TestEnv, DEFAULT_SURI};
+use anyhow::Result;
 use pop_mcp_server::tools::call::contract::{call_contract, CallContractParams};
 
 #[test]
 fn call_contract_nonexistent_path_fails() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
-
-    // Call with a non-existent contract path - this will definitely fail
+    let env = TestEnv::new()?;
     let params = CallContractParams {
         path: "/nonexistent/path/to/contract".to_string(),
         contract: "0x0000000000000000000000000000000000000000".to_string(),
@@ -19,73 +16,69 @@ fn call_contract_nonexistent_path_fails() -> Result<()> {
         url: None,
     };
 
-    let result = call_contract(&executor, params)?;
+    let result = call_contract(env.executor(), params)?;
     assert!(is_error(&result));
     assert!(text(&result)?.contains("Contract call failed"));
     Ok(())
 }
 
 #[test]
 fn call_contract_get_and_flip_mutates_state() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
-
-    let mut contract = Contract::with_context(ctx, &executor, "call_test")?;
-    contract.build(&executor)?;
-    contract.deploy(&executor, Some("new"), Some("false"))?;
-    let addr = contract
-        .address
-        .as_ref()
-        .ok_or_else(|| anyhow!("Missing contract address after deploy"))?
-        .to_string();
-    let node_url = contract
-        .node_url()
-        .ok_or_else(|| anyhow!("Missing node URL after deploy"))?
-        .to_string();
+    let env = TestEnv::new()?;
+    let mut contract = Contract::new()?;
+    contract.deploy("new", "false")?;
 
-    // Initial get - should return false (initial state)
-    let params = CallContractParams {
-        path: contract.path.to_string_lossy().to_string(),
-        contract: addr.clone(),
-        message: "get".to_string(),
-        args: None,
-        value: None,
-        execute: None,
-        suri: Some(DEFAULT_SURI.to_string()),
-        url: Some(node_url.clone()),
-    };
+    let url = InkNode::url()?.to_string();
+    let addr = contract.address().to_string();
+    let path = contract.path.display().to_string();
 
-    let result = call_contract(&executor, params)?;
+    // Initial get - should return false
+    let result = call_contract(
+        env.executor(),
+        CallContractParams {
+            path: path.clone(),
+            contract: addr.clone(),
+            message: "get".to_string(),
+            args: None,
+            value: None,
+            execute: None,
+            suri: Some(DEFAULT_SURI.to_string()),
+            url: Some(url.clone()),
+        },
+    )?;
     assert!(is_success(&result));
     assert!(text(&result)?.contains("false"));
 
-    // Call flip to mutate state
-    let flip_params = CallContractParams {
-        path: contract.path.to_string_lossy().to_string(),
-        contract: addr.clone(),
-        message: "flip".to_string(),
-        args: None,
-        value: None,
-        execute: Some(true),
-        suri: Some(DEFAULT_SURI.to_string()),
-        url: Some(node_url.clone()),
-    };
-    let flip_result = call_contract(&executor, flip_params)?;
+    // Flip to mutate state
+    let flip_result = call_contract(
+        env.executor(),
+        CallContractParams {
+            path: path.clone(),
+            contract: addr.clone(),
+            message: "flip".to_string(),
+            args: None,
+            value: None,
+            execute: Some(true),
+            suri: Some(DEFAULT_SURI.to_string()),
+            url: Some(url.clone()),
+        },
+    )?;
     assert!(is_success(&flip_result));
-    assert!(!text(&flip_result)?.is_empty());
 
-    // Call get again - should now return true
-    let get_params = CallContractParams {
-        path: contract.path.to_string_lossy().to_string(),
-        contract: addr,
-        message: "get".to_string(),
-        args: None,
-        value: None,
-        execute: None,
-        suri: Some(DEFAULT_SURI.to_string()),
-        url: Some(node_url),
-    };
-    let get_result = call_contract(&executor, get_params)?;
+    // Get again - should return true
+    let get_result = call_contract(
+        env.executor(),
+        CallContractParams {
+            path,
+            contract: addr,
+            message: "get".to_string(),
+            args: None,
+            value: None,
+            execute: None,
+            suri: Some(DEFAULT_SURI.to_string()),
+            url: Some(url),
+        },
+    )?;
     assert!(is_success(&get_result));
     assert!(text(&get_result)?.contains("true"));
     Ok(())

tests/tools/call/contract.rs

@@ -1,58 +1,46 @@
 use crate::common::{
     is_port_in_use, is_success, parse_pids, text, texts, wait_for_port_closed, ws_port_from_url,
-    TestContext,
+    TestEnv,
 };
 use anyhow::Result;
 use pop_mcp_server::tools::clean::{clean_nodes, CleanNodesParams};
 use pop_mcp_server::tools::up::chain::{up_ink_node, UpInkNodeParams};
 use serial_test::serial;
 
-/// Ports used by this test (different from shared node on 9945/8546 and defaults 9944/8545).
+/// Ports for lifecycle test (different from shared node on 9945/8546).
 const TEST_INK_PORT: u16 = 9946;
 const TEST_ETH_PORT: u16 = 8547;
 
-/// Test node lifecycle: launch ink-node, verify it's running, then clean it up.
-///
-/// This test covers both `up_ink_node` and `clean_nodes` functionality:
-/// - Verifies `up_ink_node` launches successfully and returns URL + PIDs
-/// - Verifies the node is actually listening on the expected port
-/// - Verifies `clean_nodes` terminates the node processes
-/// - Verifies the port is released after cleanup
-///
-/// Uses explicit ports (9944/8545) which don't conflict with the shared test node
-/// that runs on different ports (9945/8546).
 #[test]
 #[serial]
 fn up_ink_node_and_clean_nodes_lifecycle() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
+    let env = TestEnv::new()?;
 
-    // Launch ink-node with explicit ports
+    // Launch ink-node
     let result = up_ink_node(
-        &executor,
+        env.executor(),
         UpInkNodeParams {
             ink_node_port: Some(TEST_INK_PORT),
             eth_rpc_port: Some(TEST_ETH_PORT),
         },
     )?;
     if !is_success(&result) {
-        let err = text(&result).unwrap_or_else(|_| "unknown error".to_string());
-        panic!("up_ink_node failed: {}", err);
+        panic!("up_ink_node failed: {}", text(&result)?);
     }
 
-    // Verify URL is returned and node is listening
+    // Verify URL and node is listening
     let url = text(&result)?;
     assert!(url.starts_with("ws://"));
     let port = ws_port_from_url(&url)?;
     assert!(is_port_in_use(port));
 
-    // Parse PIDs from output
+    // Parse PIDs
     let pid_output = texts(&result).join("\n");
     let pids = parse_pids(&pid_output)?;
     assert!(!pids.is_empty());
 
-    // Clean up the node
-    let result = clean_nodes(&executor, CleanNodesParams { pids })?;
+    // Clean up
+    let result = clean_nodes(env.executor(), CleanNodesParams { pids })?;
     assert!(is_success(&result));
 
     // Verify port is released

tests/tools/clean.rs

@@ -1,30 +1,28 @@
-use crate::common::{is_error, is_success, text, TestContext};
+use crate::common::{is_error, is_success, text, TestEnv};
 use anyhow::Result;
 use pop_mcp_server::tools::convert::{convert_address, ConvertAddressParams};
 
 #[test]
 fn convert_ethereum_to_substrate() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
+    let env = TestEnv::new()?;
     let params = ConvertAddressParams {
         address: "0x742d35Cc6634C0532925a3b844Bc454e4438f44e".to_string(),
     };
 
-    let result = convert_address(&executor, params)?;
+    let result = convert_address(env.executor(), params)?;
     assert!(is_success(&result));
     assert!(!text(&result)?.is_empty());
     Ok(())
 }
 
 #[test]
 fn convert_substrate_to_ethereum() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
+    let env = TestEnv::new()?;
     let params = ConvertAddressParams {
         address: "13dKz82CEiU7fKfhfQ5aLpdbXHApLfJH5Z6y2RTZpRwKiNhX".to_string(),
     };
 
-    let result = convert_address(&executor, params)?;
+    let result = convert_address(env.executor(), params)?;
     assert!(is_success(&result));
     assert!(text(&result)?
         .to_lowercase()
@@ -34,13 +32,12 @@ fn convert_substrate_to_ethereum() -> Result<()> {
 
 #[test]
 fn convert_invalid_address_fails() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
+    let env = TestEnv::new()?;
     let params = ConvertAddressParams {
         address: "not_a_valid_address".to_string(),
     };
 
-    let result = convert_address(&executor, params)?;
+    let result = convert_address(env.executor(), params)?;
     assert!(is_error(&result));
     assert!(text(&result)?.contains("Address conversion failed"));
     Ok(())

tests/tools/convert.rs

@@ -1,12 +1,11 @@
-use crate::common::{is_success, TestContext};
+use crate::common::{is_success, TestEnv};
 use anyhow::Result;
 use pop_mcp_server::tools::install::{check_pop_installation, CheckPopInstallationParams};
 
 #[test]
 fn check_pop_installation_succeeds() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
-    let result = check_pop_installation(&executor, CheckPopInstallationParams {})?;
+    let env = TestEnv::new()?;
+    let result = check_pop_installation(env.executor(), CheckPopInstallationParams {})?;
     assert!(is_success(&result));
     Ok(())
 }

tests/tools/install.rs

@@ -1,21 +1,20 @@
-use crate::common::{is_error, is_success, text, TestContext};
+use crate::common::{is_error, is_success, text, TestEnv};
 use anyhow::Result;
 use pop_mcp_server::tools::new::contract::{create_contract, CreateContractParams};
 
 #[test]
 fn create_contract_standard_template_creates_files() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
+    let env = TestEnv::new()?;
 
     let contract_name = "test_contract";
-    let contract_path = ctx.workdir.join(contract_name);
+    let contract_path = env.workdir().join(contract_name);
 
     let params = CreateContractParams {
         name: contract_name.to_string(),
         template: "standard".to_string(),
     };
 
-    let result = create_contract(&executor, params)?;
+    let result = create_contract(env.executor(), params)?;
     assert!(is_success(&result));
 
     let output = text(&result)?;
@@ -28,26 +27,24 @@ fn create_contract_standard_template_creates_files() -> Result<()> {
 
 #[test]
 fn create_contract_invalid_name_with_hyphen_fails_validation() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
+    let env = TestEnv::new()?;
     let params = CreateContractParams {
         name: "invalid-name".to_string(),
         template: "standard".to_string(),
     };
-    let result = create_contract(&executor, params);
+    let result = create_contract(env.executor(), params);
     assert!(result.is_err());
     Ok(())
 }
 
 #[test]
 fn create_contract_nonexistent_template_fails() -> Result<()> {
-    let ctx = TestContext::new()?;
-    let executor = ctx.executor()?;
+    let env = TestEnv::new()?;
     let params = CreateContractParams {
         name: "test_contract".to_string(),
         template: "non_existing".to_string(),
     };
-    let result = create_contract(&executor, params)?;
+    let result = create_contract(env.executor(), params)?;
     assert!(is_error(&result));
     assert!(text(&result)?.starts_with("Failed to create contract:"));
     Ok(())