Improve relative path handling for create_app_fixture

Enhanced the logic for computing and rewriting relative paths from test files to app files in ShinyTestGenerator. Updated documentation and prompt instructions to clarify path requirements and added more robust regex and logging for fixture path replacement. This ensures that generated tests always use correct relative paths, never absolute paths, regardless of directory structure.

Author: karangattu

shiny/pytest/_generate/_data/_prompts/SYSTEM_PROMPT_testing.md

@@ -9,12 +9,14 @@ For non-Shiny Python code, respond: "This framework is for Shiny for Python only
 
 1. **Dynamic App File**: When generating code that uses `create_app_fixture`, follow these rules:
    - Use the exact filename provided in the prompt.
-   - If the test file is under `app_dir/tests`, make the app path relative to the tests directory.
-
-   - ✅ `app = create_app_fixture(["../app.py"])`
-   - ❌ `app = create_app_fixture(["app.py"])`
-
-   - If the provided filename is in a different path, adjust the path accordingly while keeping it relative.
+   - ALWAYS make paths relative from the test file directory to the app file.
+   - For tests in `app_dir/tests` and app in `app_dir/app.py`:
+     - ✅ `app = create_app_fixture(["../app.py"])`
+     - ❌ `app = create_app_fixture(["app.py"])`
+   - For tests in `tests/subdir` and app in `apps/subdir/app.py`:
+     - ✅ `app = create_app_fixture(["../../apps/subdir/app.py"])`
+   - NEVER use absolute paths.
+   - Calculate the correct relative path based on the test file location and app file location.
 
 2. **Controller Classes Only**: Always use official controllers, never `page.locator()`
    - ✅ `controller.InputSlider(page, "my_slider")`
@@ -36,12 +38,13 @@ For non-Shiny Python code, respond: "This framework is for Shiny for Python only
    - ❌ `selectize.set("")`
 
 7. **Skip icons**: Do not test icon functionality i.e. using tests like `expect_icon("icon_name")`.
+   - ❌ `btn2.expect_icon("fa-solid fa-shield-halved")`
 
 8. **Skip plots**: Do not test any OutputPlot content or functionality i.e. using `OutputPlot` controller.
     - ❌ plot1 = controller.OutputPlot(page, "my_plot_module-plot1")
     - ❌ plot1.expect_title("Random Scatter Plot")
 
-9. **Keyword-Only Args**: Always pass every argument as a keyword for every controller method.
+9.  **Keyword-Only Args**: Always pass every argument as a keyword for every controller method.
    - ✅  `expect_cell(value="0", row=1, col=2)`
    - ❌  `expect_cell("0", 1, 2)`
 
@@ -51,20 +54,22 @@ For non-Shiny Python code, respond: "This framework is for Shiny for Python only
 
 ### Checkbox Group
 ```python
-# app_checkbox.py
+# apps/app_checkbox.py
 from shiny.express import input, ui, render
 ui.input_checkbox_group("basic", "Choose:", ["A", "B"], selected=["A"])
 @render.text
 def output(): return f"Selected: {input.basic()}"
 
-# test_app_checkbox.py
+# apps/test_app_checkbox.py
+
 from playwright.sync_api import Page
 from shiny.playwright import controller
 from shiny.pytest import create_app_fixture
+from shiny.run import ShinyAppProc
 
 app = create_app_fixture(["app_checkbox.py"])
 
-def test_checkbox(page: Page, app) -> None:
+def test_checkbox(page: Page, app: ShinyAppProc) -> None:
     page.goto(app.url)
     basic = controller.InputCheckboxGroup(page, "basic")
     output = controller.OutputText(page, "output")
@@ -87,8 +92,16 @@ def test_checkbox(page: Page, app) -> None:
 from shiny.express import input, ui
 ui.input_date("date1", "Date:", value="2024-01-01")
 
-# test_app_date.py
-def test_date(page: Page, app) -> None:
+# tests/test_app_date.py
+from playwright.sync_api import Page
+from shiny.playwright import controller
+from shiny.pytest import create_app_fixture
+from shiny.run import ShinyAppProc
+
+app = create_app_fixture(["../app_date.py"])
+
+
+def test_date(page: Page, app: ShinyAppProc) -> None:
     page.goto(app.url)
     date1 = controller.InputDate(page, "date1")
 
@@ -112,7 +125,15 @@ def output(): return f"Selected: {input.select1()}"
 def _(): ui.update_selectize("select1", selected="CA")
 
 # test_app_selectize.py
-def test_selectize(page: Page, app) -> None:
+from playwright.sync_api import Page
+from shiny.playwright import controller
+from shiny.pytest import create_app_fixture
+from shiny.run import ShinyAppProc
+
+app = create_app_fixture(["app_selectize.py"])
+
+
+def test_selectize(page: Page, app: ShinyAppProc) -> None:
     page.goto(app.url)
     select1 = controller.InputSelectize(page, "select1")
     output = controller.OutputText(page, "output")

shiny/pytest/_generate/_main.py

@@ -218,7 +218,12 @@ def _compute_relative_app_path(
         self, app_file_path: Path, test_file_path: Path
     ) -> str:
         """Compute POSIX-style relative path from the test file directory to the app file."""
-        rel = os.path.relpath(str(app_file_path), start=str(test_file_path.parent))
+        # Make sure both paths are absolute
+        app_file_abs = app_file_path.resolve()
+        test_file_abs = test_file_path.resolve()
+
+        # Compute relative path from test file directory to app file
+        rel = os.path.relpath(str(app_file_abs), start=str(test_file_abs.parent))
         return Path(rel).as_posix()
 
     def _rewrite_fixture_path(self, test_code: str, relative_app_path: str) -> str:
@@ -229,27 +234,70 @@ def _rewrite_fixture_path(self, test_code: str, relative_app_path: str) -> str:
         - create_app_fixture("app.py") -> create_app_fixture("../app.py")
         Keeps other arguments intact if present.
         """
+        logging.debug(f"Rewriting fixture path to: {relative_app_path}")
+
+        # First check if create_app_fixture exists in the code
+        if "create_app_fixture" not in test_code:
+            logging.warning("No create_app_fixture found in generated test code")
+            return test_code
+
         # Pattern for list form: create_app_fixture(["app.py"]) or with spaces
         pattern_list = re.compile(
-            r"(create_app_fixture\(\s*\[\s*)(['\"])([^'\"]+)(\\2)(\\s*)([,\\]])",
+            r"(create_app_fixture\(\s*\[\s*)(['\"])([^'\"]+)(\\2)(\\s*)([,\]])",
             re.DOTALL,
         )
 
         def repl_list(m: re.Match) -> str:
+            logging.debug(
+                f"Replacing list form: '{m.group(3)}' with '{relative_app_path}'"
+            )
             return f"{m.group(1)}{m.group(2)}{relative_app_path}{m.group(2)}{m.group(5)}{m.group(6)}"
 
-        new_code, _ = pattern_list.subn(repl_list, test_code)
+        new_code, list_count = pattern_list.subn(repl_list, test_code)
+
+        if list_count > 0:
+            logging.debug(f"Replaced {list_count} list-form fixture path(s)")
 
         # Pattern for direct string form: create_app_fixture("app.py")
         pattern_str = re.compile(
-            r"(create_app_fixture\(\s*)(['\"])([^'\"]+)(\\2)(\\s*)([,\\)])",
+            r"(create_app_fixture\(\s*)(['\"])([^'\"]+)(\\2)(\\s*)([,\)])",
             re.DOTALL,
         )
 
         def repl_str(m: re.Match) -> str:
+            logging.debug(
+                f"Replacing string form: '{m.group(3)}' with '{relative_app_path}'"
+            )
             return f"{m.group(1)}{m.group(2)}{relative_app_path}{m.group(2)}{m.group(5)}{m.group(6)}"
 
-        new_code2, _ = pattern_str.subn(repl_str, new_code)
+        new_code2, str_count = pattern_str.subn(repl_str, new_code)
+
+        if str_count > 0:
+            logging.debug(f"Replaced {str_count} string-form fixture path(s)")
+
+        # If no replacements were made, there might be a pattern we're not catching
+        if list_count == 0 and str_count == 0:
+            logging.warning(
+                f"Found create_app_fixture but couldn't replace path. Code snippet: {test_code[:200]}..."
+            )
+
+            # Fallback regex with more generous pattern matching
+            fallback_pattern = re.compile(
+                r"(create_app_fixture\([^\)]*?['\"])([^'\"]+)(['\"][^\)]*?\))",
+                re.DOTALL,
+            )
+
+            def fallback_repl(m: re.Match) -> str:
+                logging.debug(
+                    f"Fallback replacement: '{m.group(2)}' with '{relative_app_path}'"
+                )
+                return f"{m.group(1)}{relative_app_path}{m.group(3)}"
+
+            new_code2, fallback_count = fallback_pattern.subn(fallback_repl, new_code)
+
+            if fallback_count > 0:
+                logging.debug(f"Fallback replaced {fallback_count} fixture path(s)")
+
         return new_code2
 
     def _create_test_prompt(self, app_text: str, app_file_name: str) -> str:
@@ -259,19 +307,22 @@ def _create_test_prompt(self, app_text: str, app_file_name: str) -> str:
             "Please only add controllers for components that already have an ID in the shiny app.\n"
             "Do not add tests for ones that do not have an existing ids since controllers need IDs to locate elements.\n"
             "and server functionality of this app. Include appropriate assertions \\n"
-            "and test cases to verify the app's behavior.\n"
-            "IMPORTANT: In the create_app_fixture call, pass a RELATIVE path from the test file's directory to the app file.\n"
-            "For example, if the test lives under a 'tests/' subfolder next to the app file, use '../"
-            + app_file_name
-            + "'. Do not use absolute paths.\n"
+            "and test cases to verify the app's behavior.\n\n"
+            "CRITICAL: In the create_app_fixture call, you MUST pass a RELATIVE path from the test file's directory to the app file.\n"
+            "For example:\n"
+            "- If test is in 'tests/test_app.py' and app is in 'app.py', use: '../app.py'\n"
+            "- If test is in 'tests/subdir/test_app.py' and app is in 'apps/subdir/app.py', use: '../../apps/subdir/app.py'\n"
+            "- Always compute the correct relative path from the test file to the app file\n"
+            "- NEVER use absolute paths or paths that aren't relative from the test location\n\n"
             "IMPORTANT: Only output the Python test code in a single code block. Do not include any explanation, justification, or extra text."
         )
 
     def _infer_app_file_path(
         self, app_code: Optional[str] = None, app_file_path: Optional[str] = None
     ) -> Path:
         if app_file_path:
-            return Path(app_file_path)
+            # Return absolute path to avoid any ambiguity
+            return Path(app_file_path).resolve()
 
         current_dir = Path.cwd()
 
@@ -280,10 +331,12 @@ def _infer_app_file_path(
             found_files.extend(current_dir.glob(pattern))
 
         if found_files:
-            return found_files[0]
+            # Return absolute path of found file
+            return found_files[0].resolve()
 
         if app_code:
-            return Path("inferred_app.py")
+            # For inferred app paths, use absolute path in current directory
+            return Path("inferred_app.py").resolve()
 
         raise FileNotFoundError(
             "Could not infer app file path. Please provide app_file_path parameter."
@@ -294,7 +347,8 @@ def _generate_test_file_path(
     ) -> Path:
         output_dir = output_dir or app_file_path.parent
         test_file_name = f"test_{app_file_path.stem}.py"
-        return output_dir / test_file_name
+        # Return absolute path for test file
+        return (output_dir / test_file_name).resolve()
 
     def generate_test(
         self,
@@ -328,12 +382,41 @@ def generate_test(
             )
 
         try:
+            # Log the paths for debugging
+            logging.info(f"App file path: {inferred_app_path}")
+            logging.info(f"Test file path: {test_file_path}")
+
             relative_app_path = self._compute_relative_app_path(
                 inferred_app_path, test_file_path
             )
+
+            logging.info(f"Computed relative path: {relative_app_path}")
+
+            # Explicitly check for app.py - this is a common problematic case
+            if relative_app_path == "app.py" and "../" not in relative_app_path:
+                logging.warning(
+                    f"Detected possibly incorrect relative path: {relative_app_path}"
+                )
+                # Force a proper relative path if needed
+                if test_file_path.parent != inferred_app_path.parent:
+                    logging.info(
+                        "Test and app are in different directories, adjusting relative path"
+                    )
+                    relative_app_path = f"../{relative_app_path}"
+                    logging.info(f"Adjusted relative path: {relative_app_path}")
+
             test_code = self._rewrite_fixture_path(test_code, relative_app_path)
-        except Exception:
-            pass
+        except Exception as e:
+            logging.error(f"Error computing relative path: {e}")
+            # Don't silently ignore - use the best path we can
+            try:
+                # Fallback: just use the absolute path as string if we can't compute relative
+                logging.warning("Falling back to using absolute path in test file")
+                test_code = self._rewrite_fixture_path(
+                    test_code, str(inferred_app_path.resolve())
+                )
+            except Exception as e2:
+                logging.error(f"Error in fallback path handling: {e2}")
 
         test_file_path.parent.mkdir(parents=True, exist_ok=True)
         test_file_path.write_text(test_code, encoding="utf-8")