remove non-functional attempt to pick a port

fix sort order

fixed small bug in broadcast states qurey

fix test

Remove lint from tests

Update tests/functional/cylc-review/05-cycles-job-fails/suite.rc

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>

Update tests/functional/cylc-review/12-cycle-counts.t

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>

Modify comment

set local platform

use python subproc for testing

Turn off cylc review by default with Cylc hub, and turn on how to document it.

Author: wxtim

.github/workflows/test.yml

@@ -53,20 +53,25 @@ jobs:
             bash
             coreutils
 
-      - name: install cylc-flow
-        uses: cylc/release-actions/install-cylc-components@v1
-        with:
-          cylc_flow: true
-          cylc_flow_opts: ''
-          editable_installations: true
+      # - name: install cylc-flow
+      #   uses: cylc/release-actions/install-cylc-components@v1
+      #   with:
+      #     cylc_flow: true
+      #     cylc_flow_opts: ''
+      #     editable_installations: true
+
+      - name: bodge
+        run: |
+          # This is a bodge to make this branch work before companion branches merged:
+          pip install -e git+https://github.com/wxtim/cylc@feat.cylc-review#egg=cylc-flow --no-deps
 
       - name: install cylc-uiserver
         run: |
           pip install -e .[all]
 
       - name: Style test
         if: startsWith(matrix.os, 'ubuntu')
-        run: flake8 cylc/uiserver/* tests/unit/* tests/integration/*
+        run: flake8 cylc/uiserver/* tests/integration/*
 
       - name: Type checking
         if: startsWith(matrix.os, 'ubuntu')
@@ -88,13 +93,13 @@ jobs:
 
           cat >> "$CONF_PATH/global.cylc" <<__HERE__
           [platforms]
-            [[_local_background_indep_tcp]]
+            [[_local_background_shared_tcp]]
               hosts = localhost
               install target = localhost
               ssh command = ssh -oBatchMode=yes -oConnectTimeout=8 -oStrictHostKeyChecking=no
           __HERE__
 
-          # Check that Cylc Review is ready to run
+          # Check that Cylc Review is installed!
           # (We're only interested if it emits an error):
           cylc review --help > /dev/null || exit 1
 
@@ -115,10 +120,6 @@ jobs:
 
       - name: Coverage report
         run: |
-          echo pwd:
-          pwd
-          echo ls:
-          ls
           coverage combine --append src/cylc-flow
           coverage xml --ignore-errors
           coverage report

cylc/uiserver/app.py

@@ -51,6 +51,18 @@
    ``c.CylcUIServer.site_authorization`` should be defined in
    ``/etc/cylc/uiserver/jupyter_config.py``, or, alternatively, via
    the environment variable ``CYLC_SITE_CONF_PATH``.
+
+Cylc Review Service
+^^^^^^^^^^^^^^^^^^^
+
+Cylc hub can be configured to automatically serve the Cylc Review static
+log-file viewer. To enable Cylc Review, add the following to your
+`jupyterhub_config.py`:
+
+.. code-block:: python
+
+   c.JupyterHub.services = [get_review_service_config()]
+
 """
 
 from concurrent.futures import ProcessPoolExecutor

cylc/uiserver/jupyter_config.py

@@ -107,5 +107,6 @@
 c.ServerApp.authorizer_class = CylcAuthorizer
 
 
-# Setup Cylc Review
-c.JupyterHub.services = [get_review_service_config()]
+# Setup Cylc Review - uncomment to start Cylc review with Cylc Hub
+# n.b. if installed with pip optional dependence [review] must be specified.
+# c.JupyterHub.services = [get_review_service_config()]

cylc/uiserver/review.py

@@ -527,9 +527,9 @@ def suites(
         elif order == "name_desc":
             data["entries"].sort(key=lambda entry: entry["name"], reverse=True)
         elif order == "time_asc":
-            data["entries"].sort(key=self._sort_summary_entries, reverse=True)
+            data["entries"].sort(key=self._sort_summary_entries, reverse=False)
         else:  # order == "time_desc"
-            data["entries"].sort(key=self._sort_summary_entries)
+            data["entries"].sort(key=self._sort_summary_entries, reverse=True)
         data["of_n_entries"] = len(data["entries"])
         if per_page:
             data["n_pages"] = data["of_n_entries"] // per_page

cylc/uiserver/review_dao.py

@@ -166,8 +166,8 @@ def get_suite_broadcast_states(self, user_name, suite_name):
         """Return broadcast states of a suite.
         [[point, name, key, value], ...]
         """
-        stmt = self.pre_select_broadcast_states(
-            self._db_init(user_name, suite_name), order="ASC")[0]
+        stmt = CylcWorkflowDAO.pre_select_broadcast_states(
+            self=self._db_init(user_name, suite_name), order="ASC")[0]
         broadcast_states = []
         for row in self._db_exec(user_name, suite_name, stmt):
             point, namespace, key, value = row

cylc/uiserver/ws.py

@@ -25,7 +25,6 @@
 from glob import glob
 import os
 from pathlib import Path
-import socket
 from typing import Annotated
 
 LOG_ROOT_TMPL = "~/.cylc/%(ns)s-%(util)s-%(host)s-%(port)s"
@@ -162,39 +161,24 @@ def get_util_home(*args):
 
 
 def get_review_service_config(
-    ports: tuple[Annotated[int, Ge(0)], Annotated[int, Ge(0)]] = (8000, 8999),
+    port: Annotated[int, Ge(0)] = 8042,
     service_root: str = 'services/cylc'
 ) -> dict:
     """Get a configuration for Cylc Review as a service.
 
     Args:
-        ports: A range of ports to attempt to make the Cylc Review service
-            available on.
+        port: The port to make the Cylc Review service available on.
         service_root: Include web service name under root of URL.
 
     Returns:
         Dictionary of settings for Cylc Review to be run as a hub service.
     """
-    port_found = False
-    for port in range(ports[0], ports[1]):
-        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
-            try:
-                s.bind(('localhost', port))
-            except Exception:
-                pass
-            else:
-                port_found = True
-                break
-
-    if not port_found:
-        print('No suitable port found.')
-
     return {
         "name": "cylc-review",
         "command": [
             "cylc", "review", "start",
             f"--port={port}",
             f"--service-root={service_root}",
         ],
-        "url": f"http://127.0.0.1:{port}/",
+        "url": f"http://0.0.0.0:{port}/",
     }

tests/functional/cylc-review/05-cycles-job-fails/suite.rc

@@ -14,6 +14,6 @@
         script = false
     [[rehab]]
         script = """
-            cylc set "${CYLC_WORKFLOW_NAME}//${CYLC_TASK_CYCLE_POINT}/loser"
+            cylc set "${CYLC_WORKFLOW_ID}//${CYLC_TASK_CYCLE_POINT}/loser"
             sleep 1
         """

tests/functional/cylc-review/08-suites-page.t

@@ -57,8 +57,8 @@ cylc_ws_json_greps "${TEST_NAME}.stdout" "${TEST_NAME}.stdout" \
     "[('page',), 1]" \
     "[('per_page',), 100]" \
     "[('of_n_entries',), 2]" \
-    "[('entries', 0, 'name'), '${PREFIX_GROUP1}a']" \
-    "[('entries', 1, 'name'), '${PREFIX_GROUP1}b']"
+    "[('entries', 0, 'name'), '${PREFIX_GROUP1}b']" \
+    "[('entries', 1, 'name'), '${PREFIX_GROUP1}a']"
 #-------------------------------------------------------------------------------
 # Data transfer output check for [abc], sort by time_asc
 TEST_NAME="${TEST_NAME_BASE}-200-curl-suites-time-asc"
@@ -69,8 +69,8 @@ cylc_ws_json_greps "${TEST_NAME}.stdout" "${TEST_NAME}.stdout" \
     "[('page',), 1]" \
     "[('per_page',), 100]" \
     "[('of_n_entries',), 2]" \
-    "[('entries', 0, 'name'), '${PREFIX_GROUP1}b']" \
-    "[('entries', 1, 'name'), '${PREFIX_GROUP1}a']"
+    "[('entries', 0, 'name'), '${PREFIX_GROUP1}a']" \
+    "[('entries', 1, 'name'), '${PREFIX_GROUP1}b']"
 #-------------------------------------------------------------------------------
 # Data transfer output check for [abc], sort by name_asc
 TEST_NAME="${TEST_NAME_BASE}-200-curl-suites-name-asc"
@@ -105,7 +105,7 @@ cylc_ws_json_greps "${TEST_NAME}.stdout" "${TEST_NAME}.stdout" \
     "[('page',), 1]" \
     "[('per_page',), 1]" \
     "[('of_n_entries',), 3]" \
-    "[('entries', 0, 'name'), '${PREFIX_GROUP2}1']"
+    "[('entries', 0, 'name'), '${PREFIX_GROUP2}3']"
 #-------------------------------------------------------------------------------
 # Data transfer output check for [1-10], page 2
 TEST_NAME="${TEST_NAME_BASE}-200-curl-suites-2-page-2"
@@ -127,7 +127,7 @@ cylc_ws_json_greps "${TEST_NAME}.stdout" "${TEST_NAME}.stdout" \
     "[('page',), 3]" \
     "[('per_page',), 1]" \
     "[('of_n_entries',), 3]" \
-    "[('entries', 0, 'name'), '${PREFIX_GROUP2}3']"
+    "[('entries', 0, 'name'), '${PREFIX_GROUP2}1']"
 #-------------------------------------------------------------------------------
 # Tidy up - note suites trivial so stop early on by themselves
 rm -fr "${HOME}/cylc-run/${TOP_LEVEL_TEST_DIR}"

tests/functional/cylc-review/12-cycle-counts.t

@@ -34,7 +34,7 @@ init_workflow "${TEST_NAME_BASE}" <<'__SUITE_RC__'
         T06 = bar[-PT6H] => baz
 [runtime]
     [[foo]]
-        script = cylc stop $CYLC_WORKFLOW_NAME bar.20100101T0000Z; sleep 5
+        script = cylc stop "${CYLC_WORKFLOW_ID}//20100101T0000Z/bar"; sleep 5
     [[bar, baz]]
         script = true
 __SUITE_RC__

tests/integration/test_cylc_review.py

@@ -16,14 +16,17 @@
 
 from contextlib import suppress
 import json
+from multiprocessing import Process
 import pytest
 import socket
-import subprocess
 import requests
 from time import sleep
 
 from cylc.flow import __version__ as CYLC_VERSION
 
+from cylc.uiserver.review import CylcReviewService
+from cylc.uiserver.ws import _ws_init
+
 
 @pytest.fixture(scope='module')
 def run_cylc_review(request):
@@ -35,18 +38,12 @@ def run_cylc_review(request):
     Yields:
         Server process, request to home page as json
     """
-
-    proc = subprocess.Popen(
-        [
-            'cylc',
-            'review',
-            'start',
-            '--port', port,
-            f'--service-root={service_root}',
-        ],
-        stdout=subprocess.PIPE,
-        stderr=subprocess.STDOUT,
-    )
+    proc = Process(target=_ws_init, kwargs={
+        'service_cls': CylcReviewService,
+        'port': port,
+        'service_root': service_root
+    })
+    proc.start()
 
     # Ensure that home-page is accessible:
     timeout_counter = 0
@@ -80,6 +77,7 @@ def test_basic_path(run_cylc_review):
         'host': socket.gethostname(),
         'cylc_version': CYLC_VERSION,
         'script': '/foo/cylc-review',
+        'jupythub_base': None,
     }
     data = json.loads(run_cylc_review[1].text)
     assert data == expect