From bc6195c973f8eb9af40f5392b8d6ccc089643264 Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Wed, 20 Oct 2021 10:10:13 +0100
Subject: [PATCH 1/3] changed the output typing of platform utility functions;
 these should either fail with an error from cylc/flow/platforms or return a
 platform.

---
 cylc/rose/platform_utils.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/cylc/rose/platform_utils.py b/cylc/rose/platform_utils.py
index c44c33b4..86eeac98 100644
--- a/cylc/rose/platform_utils.py
+++ b/cylc/rose/platform_utils.py
@@ -19,7 +19,7 @@
 """
 from optparse import Values
 from pathlib import Path
-from typing import Optional, Dict, Any
+from typing import Dict, Any
 
 from cylc.flow.config import WorkflowConfig
 from cylc.flow.rundb import CylcWorkflowDAO
@@ -29,7 +29,7 @@
 
 def get_platform_from_task_def(
     flow: str, task: str
-) -> Optional[Dict[str, Any]]:
+) -> Dict[str, Any]:
     """Return the platform dictionary for a particular task.
 
     Uses the flow definition - designed to be used with tasks
@@ -42,7 +42,7 @@ def get_platform_from_task_def(
     Returns:
         Platform Dictionary.
     """
-    flow_name, flow_file = parse_reg(flow, src=True)
+    _, flow_file = parse_reg(flow, src=True)
     config = WorkflowConfig(flow, flow_file, Values())
     # Get entire task spec to allow Cylc 7 platform from host guessing.
     task_spec = config.pcfg.get(['runtime', task])
@@ -52,7 +52,7 @@ def get_platform_from_task_def(
 
 def get_platforms_from_task_jobs(
     flow: str, cyclepoint: str
-) -> Optional[Dict[str, Any]]:
+) -> Dict[str, Any]:
     """Access flow database. Return platform for task at fixed cycle point
 
     Uses the workflow database - designed to be used with tasks where jobs
@@ -66,7 +66,7 @@ def get_platforms_from_task_jobs(
     Returns:
         Platform Dictionary.
     """
-    flow_name, flow_file = parse_reg(flow, src=True)
+    _, flow_file = parse_reg(flow, src=True)
     dbfilepath = Path(flow_file).parent / '.service/db'
     dao = CylcWorkflowDAO(dbfilepath)
     task_platform_map: Dict = {}

From 1dfd7d33bc9d65807d81dd099eab579069fcb058 Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Tue, 26 Oct 2021 10:39:46 +0100
Subject: [PATCH 2/3] Have get_platform_from_task_def to raise an error if
 platform == None

---
 cylc/rose/platform_utils.py  |  6 ++++++
 tests/test_platform_utils.py | 17 +++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/cylc/rose/platform_utils.py b/cylc/rose/platform_utils.py
index 86eeac98..1994b535 100644
--- a/cylc/rose/platform_utils.py
+++ b/cylc/rose/platform_utils.py
@@ -22,6 +22,7 @@
 from typing import Dict, Any
 
 from cylc.flow.config import WorkflowConfig
+from cylc.flow.exceptions import PlatformLookupError
 from cylc.flow.rundb import CylcWorkflowDAO
 from cylc.flow.workflow_files import parse_reg
 from cylc.flow.platforms import get_platform
@@ -47,6 +48,11 @@ def get_platform_from_task_def(
     # Get entire task spec to allow Cylc 7 platform from host guessing.
     task_spec = config.pcfg.get(['runtime', task])
     platform = get_platform(task_spec)
+    if platform is None:
+        raise PlatformLookupError(
+            'Platform lookup failed because the platform definition for'
+            f' task {task} is {task_spec["platform"]}.'
+        )
     return platform
 
 
diff --git a/tests/test_platform_utils.py b/tests/test_platform_utils.py
index fb6661c1..046c4d44 100644
--- a/tests/test_platform_utils.py
+++ b/tests/test_platform_utils.py
@@ -25,6 +25,7 @@
 from shutil import rmtree
 from subprocess import run
 from uuid import uuid4
+from cylc.flow.exceptions import PlatformLookupError
 
 from cylc.rose.platform_utils import (
     get_platform_from_task_def,
@@ -125,6 +126,8 @@ def fake_flow():
             [[qux]]
                 [[[remote]]]
                     host = cheese
+            [[kanga]]
+                platform = $(echo "myplatform")
             [[BAR]]
                 platform = milk
             [[child_of_bar]]
@@ -180,6 +183,20 @@ def test_get_platform_from_task_def(
     assert platform['name'] == expected_platform_n
 
 
+def test_get_platform_from_task_def_raises(
+    mock_glbl_cfg, fake_flow
+):
+    """Test getting platform from task definition.
+
+    This is approaching an integration test, because
+    although it's only testing one unit of Cylc Rose, that unit
+    is calling lots of Cylc Parts, which aren't mocked.
+    """
+    mock_glbl_cfg(*MOCK_GLBL_CFG)
+    with pytest.raises(PlatformLookupError, match='Platform lookup failed.*'):
+        get_platform_from_task_def(fake_flow[0], 'kanga')
+
+
 @pytest.mark.parametrize(
     'task, cycle, expect',
     [

From 3fd63041aad35d831458019565b2791ddd550d73 Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Wed, 27 Oct 2021 12:15:33 +0100
Subject: [PATCH 3/3] changed error message in response to review.

---
 cylc/rose/platform_utils.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cylc/rose/platform_utils.py b/cylc/rose/platform_utils.py
index 1994b535..eef57631 100644
--- a/cylc/rose/platform_utils.py
+++ b/cylc/rose/platform_utils.py
@@ -50,8 +50,8 @@ def get_platform_from_task_def(
     platform = get_platform(task_spec)
     if platform is None:
         raise PlatformLookupError(
-            'Platform lookup failed because the platform definition for'
-            f' task {task} is {task_spec["platform"]}.'
+            'Platform lookup failed; platform is a subshell to be evaluated: '
+            f' Task: {task}, platform: {task_spec["platform"]}.'
         )
     return platform