Merge pull request #1180 from cloudbees-oss/session-option

kohsuke · web-flow · commit 394e9af90cd1 · 2025-11-12T16:27:41.000-08:00
Allow file indirection of a session file
diff --git a/smart_tests/commands/detect_flakes.py b/smart_tests/commands/detect_flakes.py
@@ -12,7 +12,7 @@
 from smart_tests.utils.commands import Command
 from smart_tests.utils.env_keys import REPORT_ERROR_KEY
 from smart_tests.utils.exceptions import print_error_and_die
-from smart_tests.utils.session import get_session
+from smart_tests.utils.session import SessionId, get_session
 from smart_tests.utils.smart_tests_client import SmartTestsClient
 from smart_tests.utils.tracking import Tracking, TrackingClient
 from smart_tests.utils.typer_types import ignorable_error
@@ -35,12 +35,7 @@ class DetectFlakes(TestPathWriter):
     def __init__(
             self,
             app: Application,
-            session: Annotated[str, typer.Option(
-                "--session",
-                help="In the format builds/<build-name>/test_sessions/<test-session-id>",
-                metavar="SESSION",
-                required=True
-            )],
+            session: Annotated[SessionId, SessionId.as_option()],
             retry_threshold: Annotated[DetectFlakesRetryThreshold, typer.Option(
                 "--retry-threshold",
                 help="Thoroughness of how \"flake\" is detected",
@@ -80,7 +75,7 @@ def run(self):
                 "detect-flake",
                 params={
                     "confidence": self.retry_threshold.value.upper(),
-                    "session-id": os.path.basename(self.session),
+                    "session-id": self.session.test_part,
                     "test-runner": self.app.test_runner,
                 })
 
diff --git a/smart_tests/commands/record/attachment.py b/smart_tests/commands/record/attachment.py
@@ -3,7 +3,7 @@
 import click
 
 import smart_tests.args4p.typer as typer
-from smart_tests.utils.session import get_session
+from smart_tests.utils.session import SessionId, get_session
 
 from ... import args4p
 from ...app import Application
@@ -14,11 +14,7 @@
 @args4p.command(help="Record attachment information")
 def attachment(
     app: Application,
-    session: Annotated[str, typer.Option(
-        "--session",
-        help="test session name",
-        required=True
-    )],
+    session: Annotated[SessionId, SessionId.as_option()],
     attachments: Annotated[List[str], typer.Argument(
         multiple=True,
         help="Attachment files to upload"
@@ -33,7 +29,7 @@ def attachment(
             try:
                 with open(a, mode='rb') as f:
                     res = client.request(
-                        "post", f"{session}/attachment", compress=True, payload=f,
+                        "post", session.subpath('attachment'), compress=True, payload=f,
                         additional_headers={"Content-Disposition": f"attachment;filename=\"{a}\""})
                     res.raise_for_status()
             except OSError as e:
diff --git a/smart_tests/commands/record/tests.py b/smart_tests/commands/record/tests.py
@@ -17,7 +17,7 @@
 import smart_tests.args4p.typer as typer
 from smart_tests.utils.authentication import ensure_org_workspace
 from smart_tests.utils.env_keys import REPORT_ERROR_KEY
-from smart_tests.utils.session import get_session, parse_session
+from smart_tests.utils.session import SessionId, get_session
 from smart_tests.utils.tracking import Tracking, TrackingClient
 
 from ...app import Application
@@ -70,8 +70,7 @@ class RecordTests:
     test_session_id: int
 
     # session is generated by `smart-tests record session` command
-    # the session format is `builds/<BUILD_NUMBER>/test_sessions/<TEST_SESSION_ID>`
-    session: str
+    session: SessionId
 
     # This function, if supplied, is used to build a test path
     # that uniquely identifies a test case
@@ -125,11 +124,7 @@ def parse(report: str) -> Generator[CaseEventType, None, None]:
     def __init__(
             self,
             app: Application,
-            session: Annotated[str, typer.Option(
-                "--session",
-                help="In the format builds/<build-name>/test_sessions/<test-session-id>",
-                required=True
-            )],
+            session: Annotated[SessionId, SessionId.as_option()],
             base_path: Annotated[Path | None, typer.Option(
                 "--base",
                 help="(Advanced) base directory to make test names portable",
@@ -213,7 +208,7 @@ def __init__(
             self.client.print_exception_and_recover(e)
             # To prevent users from stopping the CI pipeline, the cli exits with a
             # status code of 0, indicating that the program terminated successfully.
-            build_name, test_session_id = parse_session(session)
+            build_name, test_session_id = session.build_part, session.test_part
             exit(0)
 
         self.reports: List[str] = []
@@ -322,7 +317,7 @@ def payload(
 
         def send(payload: Dict[str, Union[str, List]]) -> None:
             res = self.client.request(
-                "post", f"{self.session}/events", payload=payload, compress=True)
+                "post", self.session.subpath("events"), payload=payload, compress=True)
             res.raise_for_status()
 
             nonlocal is_observation
@@ -444,15 +439,15 @@ def recorded_result() -> Tuple[int, int, int, float]:
 INVALID_TIMESTAMP = datetime.datetime.fromtimestamp(0)
 
 
-def get_record_start_at(session: str, client: SmartTestsClient):
+def get_record_start_at(session: SessionId, client: SmartTestsClient):
     """
     Determine the baseline timestamp to be used for up-to-date checks of report files.
     Only files newer than this timestamp will be collected.
 
     Based on the thinking that if a build doesn't exist tests couldn't have possibly run, we attempt
     to use the timestamp of a build, with appropriate fallback.
     """
-    build_name, _ = parse_session(session)
+    build_name = session.build_part
 
     sub_path = f"builds/{build_name}"
 
diff --git a/smart_tests/commands/subset.py b/smart_tests/commands/subset.py
@@ -18,7 +18,7 @@
 from smart_tests.utils.authentication import get_org_workspace
 from smart_tests.utils.commands import Command
 from smart_tests.utils.exceptions import print_error_and_die
-from smart_tests.utils.session import get_session, parse_session
+from smart_tests.utils.session import SessionId, get_session
 from smart_tests.utils.tracking import Tracking, TrackingClient
 
 from ..app import Application
@@ -98,12 +98,7 @@ class Subset(TestPathWriter):
     def __init__(
             self,
             app: Application,
-            session: Annotated[str, typer.Option(
-                "--session",
-                help="In the format builds/<build-name>/test_sessions/<test-session-id>",
-                metavar="SESSION",
-                required=True
-            )],
+            session: Annotated[SessionId, SessionId.as_option()],
             target: Annotated[Percentage | None, typer.Option(
                 type=parse_percentage,
                 help="subsetting target from 0% to 100%"
@@ -213,7 +208,7 @@ def warn(msg: str):
             else:
                 # not to block pipeline, parse session and use it
                 self.client.print_exception_and_recover(e, "Warning: failed to check test session")
-                self.build_name, self.session_id = parse_session(session)
+                self.build_name, self.session_id = session.build_part, session.test_part
 
         if is_get_tests_from_guess and is_get_tests_from_previous_sessions:
             print_error_and_die(
diff --git a/smart_tests/utils/fail_fast_mode.py b/smart_tests/utils/fail_fast_mode.py
@@ -4,6 +4,7 @@
 import click
 
 from .commands import Command
+from .session import SessionId
 
 _fail_fast_mode_cache: Optional[bool] = None
 
@@ -30,7 +31,7 @@ def warn_and_exit_if_fail_fast_mode(message: str):
 
 class FailFastModeValidateParams:
     def __init__(self, command: Command, build: Optional[str] = None, is_no_build: bool = False,
-                 test_suite: Optional[str] = None, session: Optional[str] = None,
+                 test_suite: Optional[str] = None, session: Optional[SessionId] = None,
                  links: Sequence[Tuple[str, str]] = (), is_observation: bool = False,
                  flavor: Sequence[Tuple[str, str]] = ()):
         self.command = command
diff --git a/smart_tests/utils/session.py b/smart_tests/utils/session.py
@@ -4,11 +4,11 @@
 import re
 import sys
 from dataclasses import dataclass
-from typing import Tuple
 
 import click
 from requests import HTTPError
 
+from smart_tests.args4p import typer
 from smart_tests.args4p.exceptions import BadCmdLineException
 from smart_tests.utils.smart_tests_client import SmartTestsClient
 from smart_tests.utils.tracking import Tracking
@@ -23,18 +23,58 @@ class TestSession:
     name: str | None = None
 
 
-def get_session(session: str, client: SmartTestsClient) -> TestSession:
-    build_name, test_session_id = parse_session(session)
-
-    subpath = f"builds/{build_name}/test_sessions/{test_session_id}"
-    res = client.request("get", subpath)
+class SessionId:
+    '''Represents the user-specific test session via the --session option.'''
+
+    def __init__(self, id: str):
+        '''This is the method in which we parse the user input, so be defensive'''
+        if id.startswith('@'):
+            file_path = id[1:]
+            try:
+                with open(file_path, 'r') as f:
+                    id = f.read().strip()
+            except FileNotFoundError:
+                raise BadCmdLineException(f"Session file '{file_path}' not found.")
+            except IOError as e:
+                raise BadCmdLineException(f"Error reading session file '{file_path}': {e}")
+
+        match = re.match(r"builds/([^/]+)/test_sessions/(.+)", id)
+
+        if match:
+            self.id = id
+            self.build_part = match.group(1)
+            self.test_part = int(match.group(2))
+        else:
+            raise BadCmdLineException(
+                f"Invalid session ID. Expecting the output from 'smart-tests record session', but got '{id}'")
+
+    def __str__(self):
+        return self.id
+
+    def subpath(self, endpoint: str) -> str:
+        return f"{self.id}/{endpoint}"
+
+    @staticmethod
+    def as_option():
+        '''To promote consistency of the --session option across commands, use this to define an option.'''
+        return typer.Option(
+            "--session",
+            help="Session ID obtained by calling 'smart-tests record session'. It also accepts '@path/to/file' if the session ID is stored in a file ",  # noqa E501
+            required=True,
+            metavar="SESSION",
+            type=SessionId,
+        )
+
+
+def get_session(session: SessionId, client: SmartTestsClient) -> TestSession:
+    res = client.request("get", session.id)
 
     try:
         res.raise_for_status()
     except HTTPError as e:
         if e.response.status_code == 404:
             # TODO(Konboi): move subset.print_error_and_die to util and use it
-            msg = f"Session {session} was not found. Make sure to run `smart-tests record session --build {build_name}` before you run this command"  # noqa E501
+            msg = f"Session {session} was not found."
             click.secho(msg, fg='red', err=True)
             if client.tracking_client:
                 client.tracking_client.send_error_event(event_name=Tracking.ErrorEvent.USER_ERROR, stack_trace=msg)
@@ -50,24 +90,3 @@ def get_session(session: str, client: SmartTestsClient) -> TestSession:
         observation_mode=test_session.get("isObservation"),
         name=test_session.get("name"),
     )
-
-
-def parse_session(session: str) -> Tuple[str, int]:
-    """Parse session to extract build name and test session id.
-
-    Args:
-        session: Session in format "builds/{build_name}/test_sessions/{test_session_id}"
-
-    Returns:
-        Tuple of (build_name, test_session_id)
-
-    Raises:
-        ValueError: If session_id format is invalid
-    """
-    match = re.match(r"builds/([^/]+)/test_sessions/(.+)", session)
-
-    if match:
-        return match.group(1), int(match.group(2))
-    else:
-        raise BadCmdLineException(
-            f"Invalid session ID format: {session}. Expected format: builds/{{build_name}}/test_sessions/{{test_session_id}}")
diff --git a/tests/cli_test_case.py b/tests/cli_test_case.py
@@ -45,6 +45,10 @@ def setUp(self):
         # not to use cached configuration between tests
         responses.reset()
 
+        # This is a bad idea -- to have one place where all the mocked responses are defined for all the test cases.
+        # These fixtures should be moved closer to where they are used, as much as possible. It'll take time to clean
+        # this up, but at least let's not add new ones here.
+        # (Kohsuke 2025-11-12)
         responses.add(
             responses.POST,
             f"{get_base_url()}/intake/organizations/{self.organization}/workspaces/{self.workspace}"
diff --git a/tests/commands/record/test_tests.py b/tests/commands/record/test_tests.py
@@ -2,6 +2,7 @@
 import json
 import os
 import sys
+import tempfile
 from pathlib import Path
 from unittest import mock
 
@@ -18,7 +19,13 @@ class TestsTest(CliTestCase):
     @responses.activate
     @mock.patch.dict(os.environ, {"SMART_TESTS_TOKEN": CliTestCase.smart_tests_token})
     def test_with_group_name(self):
-        result = self.cli('record', 'tests', 'maven', '--session', self.session, '--group', 'hoge',
+        # also testing the use of @session_file syntax here
+        with tempfile.NamedTemporaryFile(mode="w+", delete=False) as tmp:
+            tmp.write(self.session)
+            tmp.flush()
+            session_file = tmp.name
+
+        result = self.cli('record', 'tests', 'maven', '--session', f'@{session_file}', '--group', 'hoge',
                           str(self.report_files_dir) + "**/reports/")
 
         self.assert_success(result)
diff --git a/tests/utils/test_session.py b/tests/utils/test_session.py
@@ -4,7 +4,7 @@
 import responses
 
 from smart_tests.utils.http_client import get_base_url
-from smart_tests.utils.session import TestSession, get_session
+from smart_tests.utils.session import SessionId, TestSession, get_session
 from smart_tests.utils.smart_tests_client import SmartTestsClient
 from tests.cli_test_case import CliTestCase
 
@@ -27,7 +27,7 @@ def test_get_session(self):
             },
             status=200)
 
-        test_session = get_session(self.session, client)
+        test_session = get_session(SessionId(self.session), client)
         self.assertEqual(test_session, TestSession(
             id=self.session_id,
             build_id=456,
@@ -44,5 +44,5 @@ def test_get_session(self):
             status=404)
 
         with self.assertRaises(SystemExit) as cm:
-            get_session(self.session, client)
+            get_session(SessionId(self.session), client)
         self.assertEqual(cm.exception.code, 1)
