From a72e8628eb4cf0f1fb45c51e15d777fe2db83519 Mon Sep 17 00:00:00 2001 From: David Perl Date: Tue, 22 Sep 2026 17:15:46 +0200 Subject: [PATCH] add debugging doc and tests for file path propagation --- docs/rotation_filename_path.md | 387 ++++++++++++++++ .../test_rotation_filename_walkthrough.py | 432 ++++++++++++++++++ 2 files changed, 819 insertions(+) create mode 100644 docs/rotation_filename_path.md create mode 100644 tests/unit/test_rotation_filename_walkthrough.py diff --git a/docs/rotation_filename_path.md b/docs/rotation_filename_path.md new file mode 100644 index 00000000..7a5f919c --- /dev/null +++ b/docs/rotation_filename_path.md @@ -0,0 +1,387 @@ +# Rotation scan filename: origin to detector + +Trace of how the rotation-scan file path is built and transformed, from its origin +(GUI or aaredb) down to the JFJoch `DatasetSettings` that name the file on disk. + +All line numbers are from the working tree at the time of writing. + +> **Naming correction.** There is no `_execute_rotation_request` in `daq.py`. +> The method both paths converge on is **`_execute_rotation_sequence`** +> (`daq.py:1431`), called from `measure_rotation` (`daq.py:2071`) and from +> `measure` (`daq.py:2654`). + +--- + +## 1. Summary + +Two independent origins produce a `RotationScanRequest.file_prefix` +(`aarecommon/models/rotation_scan.py:13`, `str | None`): + +| | Origin A — manual (GUI) | Origin B — automation | +|---|---|---| +| Entry | `FilePathPanel` widgets | `AareDAQ.measure()` | +| Built in | `file_path_panel.py:145` | `daq.py:2469`, `daq.py:2647` | +| Sample data from | aaredb via `DAQStatusModel.sample` | aaredb via `SampleShortInfo` argument | +| Transport | `POST /scan/rotation` | in-process | +| Converges at | `measure_rotation` → `_execute_rotation_sequence` | `_execute_rotation_sequence` | + +Both then pass unchanged through `_execute_rotation_sequence` and the rotation +service, and receive their **only server-side path transformation** inside +`jfjoch._format_dataset_settings` (`jfjoch.py:189`), which prepends a +pgroup- and scan-kind-dependent `data_folder`. + +--- + +## 2. Origin A — manual rotation and screening from the GUI + +### 2.1 Widget input (`file_path_panel.py`) + +Three widgets feed the name: + +- `directory_edit` — directory template, may contain `{prefix}` and macros. + **Defaults to `"{date}/{puck}/{pos}"`** (line 64) — see §6.6, `{pos}` is never substituted +- `file_prefix_edit` — prefix template, **defaults to the literal `"{sample}"`** (line 77) +- `run_number_edit` — integer run number, 1..999 + +### 2.2 `update_filename()` (`file_path_panel.py:145`) + +``` +dir_name = directory_edit.text() +file_prefix = file_prefix_edit.text() + +dir_name = dir_name.replace("{prefix}", file_prefix) # prefix substituted into directory + +base = "" if dir_name == "" + = dir_name if dir_name ends with "/" + = dir_name + "/" otherwise + +base += "run" if file_prefix == "" else file_prefix # empty prefix -> literal "run" +``` + +The name is then exactly what the fields say — `update_filename` does **no** +collision handling. The result is stored **without** any scan-kind component: + +```python +self._filename = self._expand_macros(base, run_number) # line 165 +self.path_updated.emit(self._filename) # line 172 +``` + +The comment at line 164 is explicit: *"Store the GUI's base (without applying +scan-kind transforms) for wiring into requests later."* + +Collision handling lives in the click-time guard `file_path_error_box(scan_kind)` +(`file_path_panel.py:240`), which the Run buttons call. It sets `_scan_kind`, +refreshes the name for that kind, and tests it with `_exists_for_run()` → +`_effective_dataset_base()` (line 120), which checks +`//raw//_master.h5`. On a clash it bumps the run +number by one, warns the user, and **refuses the run** (returns `False`), so the +next click re-checks. Nothing is silently skipped. + +### 2.3 `_expand_macros(base, rn)` (`file_path_panel.py:106`) + +Appends the zero-padded run number, then substitutes macros: + +``` +name = f"{base}_{rn:03d}" +``` + +| Macro | Replaced with | +|---|---| +| `{date}` | `self._formatted_date` (`yyyymmdd`) | +| `{sample}`, `{CrystalName}` | `self._sample_name` | +| `{puck}` | `self._puck_name` | +| `{position}` | `f"{self._puck_pos:02d}"` | +| `{sample_id}` | `self._sample_id` | +| `{beamline}` | `self._beamline` | +| `{prefix}` | `f"{puck_name}/{puck_pos:02d}/{sample_name}"` | + +There is **no `{pos}` entry**, although both tooltips advertise one +(lines 69 and 82) and the default directory template uses it — see §6.6. + +With an empty directory and `file_prefix_edit == "{sample}"` this yields: + +``` +_001 +``` + +With the *shipped* defaults (`directory_edit == "{date}/{puck}/{pos}"`) it yields: + +``` +20260922/PUCK1/{pos}/xtal7_001 +``` + +### 2.4 Distribution (`data_collection_settings.py:190-192`) + +```python +self.file_path_panel.path_updated.connect(raster_mgr.update_filename) +self.file_path_panel.path_updated.connect(self.screening.update_filename) +self.file_path_panel.path_updated.connect(self.simple.update_filename) +``` + +Each consumer stores it verbatim as `self._filename`. + +### 2.5 Scan-kind prefix applied at request build time + +`rotation_data_collection.py:33-40`: + +```python +def add_screening_to_path(path): return "screening" / Path(path) +def add_data_to_path(path): return "data" / Path(path) +``` + +| Call site | Transform | Resulting `file_prefix` | +|---|---|---| +| `rotation_data_collection.py:341` (rotation) | `add_data_to_path` | `data/_001` | +| `rotation_data_collection.py:303` (screening, single image) | `add_screening_to_path` | `screening/_001` | +| `rotation_data_collection.py:315` (screening, wedges) | `add_screening_to_path` | `screening/_001` | +| `smart_rotation_panel.py:523` (smart rotation) | `add_data_to_path` | `data/_001` | + +`str()` is applied to the `PosixPath` when assigning to `file_prefix`. + +### 2.6 Transport + +`rotation_scan` signal → `daq_worker` → `POST /scan/rotation` → `server.py` +`rotation` endpoint → `daq.measure_rotation(val)`. The model is serialised and +revalidated as JSON; `file_prefix` is unchanged. + +--- + +## 3. Origin B — automation (`AareDAQ.measure`) + +### 3.1 Base prefix (`daq.py:2469`) + +```python +formatted_date = datetime.now().strftime("%Y%m%d") +sample_prefix = f"{formatted_date}/{sample.puck_name}/{sample.pin:02d}/{sample.sample_name}" +``` + +`sample` is the `SampleShortInfo` passed into `measure()`, sourced from aaredb +(via TELL/spreadsheet sync). Note this is a **slash-separated directory +hierarchy**, not a flat prefix, and is date-first — a different shape from the +GUI's `_`. + +### 3.2 Per-run token (`daq.py:2558`) + +```python +hex_string = secrets.token_hex(3) # 6 hex chars +``` + +One token is generated per `measure()` call and reused by **both** the raster and +the rotation request, so the two datasets share a run identifier. + +### 3.3 Raster leg (`daq.py:2558-2568`) + +```python +raster_params = self.get_auto_raster_params() +if raster_params.filename is not None: + sample_prefix = f"{raster_params.filename}/{sample.sample_name}" # reassigns sample_prefix + +raster_grid = RasterGridRequest( + file_prefix=sample_prefix + f"_{hex_string}", # no "data/" component + ... +) +``` + +### 3.4 Rotation leg (`daq.py:2640-2652`) + +```python +params, source = self.get_collection_params(prefer_smart=False) +... +if params.filename is not None: + sample_prefix = f"{params.filename}/{sample.sample_name}" + +rotation_request = RotationScanRequest( + file_prefix="data/" + sample_prefix + f"_{hex_string}", + ... +) +``` + +With both override branches inert (see §6.3), the automation `file_prefix` is: + +``` +data////_ +``` + +### 3.5 Where `params.filename` would come from + +`get_collection_params` (`daq.py:2396`) picks one of three `SimpleScanParameters` +(`filename: str | None = None`, `models.py:699`): + +| Source | Origin | Sets `filename`? | +|---|---|---| +| `spreadsheet_params()` | aaredb `sample.aaredb_params` | No | +| `smart_params` (`cfg.auto_params`) | GUI → `POST /scan/smart_params` → Redis `:auto_params` | No | +| `default_params` | `dispatch.default_rotation_parameters` | No — `{"exp_time_s", "dtz", "incr_omega_deg"}` only | + +--- + +## 4. Shared tail: request to detector + +### 4.1 `measure_rotation` (`daq.py:2051`) + +Logs the request, calls `_execute_rotation_sequence`, manages beamline state. +**No path transformation.** + +### 4.2 `_execute_rotation_sequence` (`daq.py:1431`) + +Clamps `exp_time_s` to `>= 0.0011`, validates parameters, delegates to +`self._create_rotation_service().run(request)`. **No path transformation.** + +### 4.3 Rotation service (`operations/rotation/service.py:118`) + +`run()` contains no reference to `file_prefix`. **No path transformation.** + +### 4.4 `jfjoch._format_dataset_settings` (`jfjoch.py:135-189`) + +The only server-side transformation. `data_folder` is chosen by request type and +the `screening` flag (`jfjoch.py:152-169`): + +| Request | `data_folder` | +|---|---| +| `RasterGridRequest` | `{pgroup}/raw/raster` | +| `RotationScanRequest`, `screening=True` | `{pgroup}/raw/screening` | +| `RotationScanRequest`, `screening=False` | `{pgroup}/raw` | + +`pgroup` is `s.sample.user`, falling back to the hard-coded `"p16371"` when +`s.sample is None` (`jfjoch.py:143-145`). + +```python +file_prefix=f"{data_folder}/{r.file_prefix}" # jfjoch.py:189 +``` + +### 4.5 Final values + +| Path | `DatasetSettings.file_prefix` | +|---|---| +| GUI rotation | `{pgroup}/raw/data/_001` | +| GUI screening | `{pgroup}/raw/screening/screening/_001` | +| GUI smart rotation | `{pgroup}/raw/data/_001` | +| Automation rotation | `{pgroup}/raw/data////_` | + +--- + +## 5. Write-back to aaredb + +`aaredb.py:195` (rotation) and `aaredb.py:258` (raster): + +```python +dataset = Datasets(filepath=r.file_prefix, status="written", written_at=...) +``` + +This records the **pre-JFJoch** value, i.e. *without* `data_folder` and without +the `/sls/mx/data` root. The database `filepath` is therefore not the path the +file was written to. + +--- + +## 6. Findings + +### 6.1 Screening prefix is applied twice + +The GUI prepends `screening/` to `file_prefix` +(`rotation_data_collection.py:303`, `:315`), and JFJoch independently selects +`data_folder = {pgroup}/raw/screening` for `screening=True` (`jfjoch.py:162`). +The final prefix contains `screening` twice: + +``` +{pgroup}/raw/screening/screening/_001 +``` + +The GUI's own preview label and collision check +(`_effective_dataset_base`, `file_path_panel.py:120`) build +`/{pgroup}/raw/screening/` — one level, not two. So for screening +the displayed path, the run-number collision check, and the written path +disagree. Rotation and raster do not have this problem: the GUI contributes +`data/` where JFJoch contributes none, and contributes nothing for raster where +JFJoch contributes `raster`. + +Worth confirming against a real screening dataset on disk before changing +anything, since the fix differs depending on which of the two is considered +authoritative. + +### 6.2 `spreadsheet_params()` never returns a file prefix + +`daq.py:1629` initialises `file_prefix = None` and no code path ever assigns it; +all four `return` statements return that `None`. The sole caller discards it: + +```python +spreadsheet_params, _file_prefix = self.spreadsheet_params() # daq.py:2397 +``` + +The `tuple[SimpleScanParameters | None, str | None]` return type and the +underscore-prefixed unpack suggest a per-sample aaredb-driven prefix was +intended but never wired up. **There is currently no aaredb origin for the +filename** — aaredb only supplies the sample identity fields that `measure()` +formats itself. + +### 6.3 Both filename override branches in `measure()` are dead + +```python +if raster_params.filename is not None: # daq.py:2562 +if params.filename is not None: # daq.py:2640 +``` + +`raster_params` comes from `get_auto_raster_params()` → `get_beamline_default_raster_params()` +(`daq.py:2307`), which constructs `SimpleScanParameters(dtz=..., exp_time_s=..., transmission=...)` +and never sets `filename`; the aaredb overlay that follows touches only exposure, +transmission and dtz. + +`params` comes from `get_collection_params()`, whose three sources all leave +`filename` at its `None` default (§3.5) — including `smart_params`, because +`SmartRotationPanel` builds its `SimpleScanParameters` at +`smart_rotation_panel.py:492` without a `filename=` argument. + +So the automation prefix is always the `sample_prefix` computed at `daq.py:2469`. + +### 6.4 `sample_prefix` leaks from the raster leg into the rotation leg + +`sample_prefix` is a single local rebound in both legs. The raster branch at +`daq.py:2562` reassigns it to `f"{raster_params.filename}/{sample.sample_name}"`, +and that reassignment is still in effect when the rotation branch reads it at +`daq.py:2647`. If `raster_params.filename` were ever set while `params.filename` +was not, the rotation dataset would silently inherit the raster-derived prefix +and lose the date/puck/pin hierarchy. + +Currently latent because both branches are dead (§6.3), but it becomes live the +moment either filename source is wired up. + +### 6.5 Database path is not the written path + +See §5. `Datasets.filepath` is missing `{pgroup}/raw[/screening|/raster]` and the +storage root, so it cannot be used to locate the file without re-deriving the +`data_folder` rules from `jfjoch.py:152-169`. + +### 6.6 The default directory template contains a macro that is never substituted + +`directory_edit` ships with the default text `"{date}/{puck}/{pos}"` +(`file_path_panel.py:64`), and `{pos}` is documented in both tooltips +(`file_path_panel.py:69`, `:82`). But `_expand_macros` (`file_path_panel.py:106-118`) +substitutes only `{position}` — there is no `{pos}` replacement. + +Replaying the shipped default through the macro table gives: + +``` +20260922/PUCK1/{pos}/xtal7_001 +``` + +The literal four characters `{pos}` become a directory component, so with stock +settings every manually collected dataset is written under a directory named +`{pos}` instead of the pin position. It propagates all the way to +`DatasetSettings.file_prefix` and into `Datasets.filepath` in aaredb. + +Either add `name.replace("{pos}", f"{self._puck_pos:02d}")` alongside the +`{position}` line, or change the default template and both tooltips to +`{position}`. The first is the safer fix, since operators may already have +`{pos}` saved in their own templates. + +### 6.7 Minor + +- `clean_filename` is disabled in `daq.py:_expand_macros` (`daq.py:1625`) with a + TODO about it stripping slashes. Nothing sanitises user-supplied text, so a + sample name or prefix containing `/`, `..` or shell-significant characters + propagates into the dataset path unaltered. +- `daq.py:1613` `_expand_macros` duplicates the GUI's macro table + (`file_path_panel.py:106`) but is **called from nowhere** — the server-side copy + is unused. The two would need to be kept in sync if it were ever wired up. +- The `"p16371"` pgroup fallback (`jfjoch.py:145`) writes into a fixed pgroup + when sample identity is missing, rather than failing. diff --git a/tests/unit/test_rotation_filename_walkthrough.py b/tests/unit/test_rotation_filename_walkthrough.py new file mode 100644 index 00000000..5328d8cf --- /dev/null +++ b/tests/unit/test_rotation_filename_walkthrough.py @@ -0,0 +1,432 @@ +"""Debugger walkthrough: how a rotation scan's filename is built. + +Companion to ``docs/rotation_filename_path.md``. These tests are not here to +catch regressions so much as to give you somewhere to put a breakpoint. Each +one arranges the minimum, then makes exactly **one** call into real production +code -- step into that call and you are inside the stage being described. + +Run one stage at a time: + + pytest tests/unit/test_rotation_filename_walkthrough.py -k expand_macros -s + +Two tests are marked ``xfail(strict=True)``: they assert what the code *should* +do, and fail because of the bugs recorded in docs sections 6.1 and 6.6. Read +their bodies to see the defect reproduced in real code; they will start passing +(and so fail as strict xfails) the moment either bug is fixed. + +Stage map, in the order data flows: + + 1. FilePathPanel._expand_macros file_path_panel.py:106 + 2. FilePathPanel.update_filename file_path_panel.py:145 + 3. add_data_to_path/_screening_ rotation_data_collection.py:33 + 4. measure() prefix construction daq.py:2469, :2647 (mirrored, see below) + 5. _format_dataset_settings jfjoch.py:133 <- the only server-side transform + 6. aaredb write-back aaredb.py:195 +""" + +from datetime import datetime + +import pytest +from aarecommon.models.rotation_scan import RotationScanRequest + +# --------------------------------------------------------------------------- +# Stage 1 -- GUI macro expansion (file_path_panel.py:106) +# --------------------------------------------------------------------------- + + +@pytest.fixture +def panel(qapp): + """A real FilePathPanel. + + Constructing it runs update_filename() once, so by the time you get here + ``panel._filename`` already holds a fully expanded name. Breakpoint in + FilePathPanel.__init__ if you want to watch that first pass. + """ + from aare.gui.panels.file_path_panel import FilePathPanel + + return FilePathPanel() + + +def test_expand_macros_substitutes_the_documented_table(panel): + """Step into _expand_macros: file_path_panel.py:106. + + Note the run number is appended *first*, then macros are replaced, so a + macro cannot expand into something containing the run number. + """ + panel._formatted_date = "20260922" + panel._sample_name = "xtal7" + panel._puck_name = "PUCK1" + panel._puck_pos = 3 + panel._sample_id = 42 + panel._beamline = "x10sa" + + expanded = panel._expand_macros("{date}/{puck}/{position}/{sample}", 1) + + assert expanded == "20260922/PUCK1/03/xtal7_001" + + +def test_expand_macros_prefix_macro_expands_to_three_levels(panel): + """``{prefix}`` is the shorthand that expands to puck/pin/sample at once.""" + panel._sample_name = "xtal7" + panel._puck_name = "PUCK1" + panel._puck_pos = 3 + + expanded = panel._expand_macros("{prefix}", 7) + + # Expanded after the run number is appended, so _007 lands on the sample. + assert expanded == "PUCK1/03/xtal7_007" + + +@pytest.mark.xfail( + strict=True, + reason=( + "docs 6.6: directory_edit ships defaulting to '{date}/{puck}/{pos}' " + "(file_path_panel.py:64) and both tooltips advertise {pos}, but " + "_expand_macros only substitutes {position}. The literal four " + "characters '{pos}' become a directory component." + ), +) +def test_expand_macros_substitutes_pos_as_advertised(panel): + """Step into _expand_macros and look for a ``{pos}`` replace -- there is none. + + This is the *shipped default* directory template, so with stock settings + every manually collected dataset is written under a directory called + literally ``{pos}``. + """ + panel._formatted_date = "20260922" + panel._sample_name = "xtal7" + panel._puck_name = "PUCK1" + panel._puck_pos = 3 + + expanded = panel._expand_macros("{date}/{puck}/{pos}/{sample}", 1) + + assert expanded == "20260922/PUCK1/03/xtal7_001" + + +# --------------------------------------------------------------------------- +# Stage 2 -- assembling the base name (file_path_panel.py:145) +# --------------------------------------------------------------------------- + + +def test_update_filename_splices_the_prefix_into_the_directory(panel): + """Step into update_filename: file_path_panel.py:145. + + Watch two things happen in order: + 1. ``{prefix}`` inside the *directory* is replaced with the prefix field + 2. a "/" is ensured between directory and prefix + + The name is then whatever the fields literally say -- update_filename does + no collision handling at all. That moved to the click-time guard exercised + in the next two tests. + """ + panel._sample_name = "xtal7" + panel.directory_edit.setText("runs/{prefix}") + panel.file_prefix_edit.setText("{sample}") + panel.run_number_edit.setValue(1) + + panel.update_filename() + + # "{prefix}" in the directory became "{sample}", which then expanded. + assert panel._filename == "runs/xtal7/xtal7_001" + + +def test_run_is_allowed_when_the_target_path_is_free(panel, monkeypatch): + """Step into file_path_error_box: file_path_panel.py:240. + + This is the click-time guard the Run buttons call. It sets the scan kind, + refreshes the name for that kind, and asks _exists_for_run about it. The + existence check is stubbed so nothing touches /sls; drop the monkeypatch to + watch it probe the real filesystem via _effective_dataset_base. + """ + monkeypatch.setattr(panel, "_exists_for_run", lambda _base: False) + panel._sample_name = "xtal7" + panel.directory_edit.setText("") + panel.file_prefix_edit.setText("{sample}") + panel.run_number_edit.setValue(1) + + may_run = panel.file_path_error_box(scan_kind="rotation") + + assert may_run is True + assert panel.run_number_edit.value() == 1 + + +def test_run_is_refused_and_the_run_number_bumped_on_a_clash(panel, monkeypatch): + """The clash branch at file_path_panel.py:249. + + Note it *refuses* the run rather than silently picking the next free + number: the run number goes up by exactly one, the user is told, and the + next click re-checks. QMessageBox is stubbed out so the test does not block + on a modal dialog. + """ + from aare.gui.panels import file_path_panel as module + + monkeypatch.setattr(panel, "_exists_for_run", lambda _base: True) + monkeypatch.setattr(module.QMessageBox, "warning", lambda *a, **k: None) + panel._sample_name = "xtal7" + panel.directory_edit.setText("") + panel.file_prefix_edit.setText("{sample}") + panel.run_number_edit.setValue(1) + + may_run = panel.file_path_error_box(scan_kind="rotation") + + assert may_run is False + assert panel.run_number_edit.value() == 2 + + +def test_update_filename_stores_the_base_without_a_scan_kind(panel): + """The stored name carries no data/ or screening/ component. + + That is deliberate (see the comment at file_path_panel.py:164) -- the scan + kind is attached later, per request, at stage 3. Note _scan_kind still + affects the *preview label* via _effective_dataset_base, which is the root + of the screening mismatch in docs 6.1. + """ + panel._sample_name = "xtal7" + panel.directory_edit.setText("") + panel.file_prefix_edit.setText("{sample}") + + panel.update_filename() + + assert not panel._filename.startswith(("data/", "screening/")) + + +# --------------------------------------------------------------------------- +# Stage 3 -- the scan-kind prefix (rotation_data_collection.py:33) +# --------------------------------------------------------------------------- + + +def test_add_data_to_path_prefixes_rotation_datasets(): + """Applied at rotation_data_collection.py:341 and smart_rotation_panel.py:523.""" + from aare.gui.panels.rotation_data_collection import add_data_to_path + + assert str(add_data_to_path("20260922/PUCK1/03/xtal7_001")) == ( + "data/20260922/PUCK1/03/xtal7_001" + ) + + +def test_add_screening_to_path_prefixes_screening_datasets(): + """Applied at rotation_data_collection.py:303 and :315. + + Compare with stage 5: JFJoch *also* routes screening into a screening + folder, which is where the duplication in docs 6.1 comes from. + """ + from aare.gui.panels.rotation_data_collection import add_screening_to_path + + assert str(add_screening_to_path("20260922/PUCK1/03/xtal7_001")) == ( + "screening/20260922/PUCK1/03/xtal7_001" + ) + + +# --------------------------------------------------------------------------- +# Stage 4 -- the automation prefix (daq.py:2469 and :2647) +# --------------------------------------------------------------------------- +# +# measure() is a ~200 line hardware sequence, so there is no seam to call here. +# The two tests below mirror its two lines verbatim so you can see the shape and +# feed it into stage 5. To watch the real thing, breakpoint daq.py:2469. + + +def test_automation_prefix_is_a_date_first_directory_hierarchy(sample_info): + """Mirrors daq.py:2469. + + Unlike the GUI's flat ``_``, automation builds a slash-separated + hierarchy led by the date. Both end up in the same ``file_prefix`` field. + """ + formatted_date = datetime.now().strftime("%Y%m%d") + + sample_prefix = ( + f"{formatted_date}/{sample_info.puck_name}/{sample_info.pin:02d}/{sample_info.sample_name}" + ) + + assert sample_prefix == f"{formatted_date}/puck1/03/sample1" + + +def test_automation_rotation_request_prepends_data_and_appends_a_token(): + """Mirrors daq.py:2647. + + The hex token is generated once per measure() call (daq.py:2558) and shared + with the raster request, so a sample's raster and rotation datasets carry + the same run identifier. + """ + request = RotationScanRequest( + file_prefix="data/" + "20260922/puck1/03/sample1" + "_a1b2c3", + exp_time_s=0.1, + incr_omega_deg=0.1, + steps=10, + dtz=200, + ) + + assert request.file_prefix == "data/20260922/puck1/03/sample1_a1b2c3" + + +def test_no_parameter_source_ever_supplies_a_filename(): + """Why the ``if params.filename is not None`` branches never fire. + + docs 6.3: get_collection_params picks one of three SimpleScanParameters and + all three leave ``filename`` at its default. get_beamline_default_raster_params + touches no ``self``, so it is callable unbound -- step into it at daq.py:2307. + """ + from aarecommon.models.models import SimpleScanParameters + + from aare.beamline_dispatch.beamline_dispatch import get_beamline_dispatch + from aare.daq.daq import AareDAQ + + raster_defaults = AareDAQ.get_beamline_default_raster_params(None) + + assert raster_defaults.filename is None + assert SimpleScanParameters().filename is None + assert "filename" not in get_beamline_dispatch().default_rotation_parameters + + +# --------------------------------------------------------------------------- +# Stage 5 -- the only server-side transform (jfjoch.py:133) +# --------------------------------------------------------------------------- +# +# _format_dataset_settings uses no `self`, so it can be called unbound with None +# and needs no detector, no network and no init. This is the most useful +# breakpoint in the whole path. + + +@pytest.fixture +def status(daq_status_factory, sample_info): + """A real DAQStatusModel carrying a sample, so pgroup resolves to its user. + + With ``sample=None`` JFJoch falls back to the hard-coded pgroup "p16371" + (jfjoch.py:143) -- worth stepping through once. + """ + return daq_status_factory(sample=sample_info) + + +def rotation_request(*, file_prefix: str, screening: bool) -> RotationScanRequest: + return RotationScanRequest( + file_prefix=file_prefix, + exp_time_s=0.1, + incr_omega_deg=0.1, + steps=10, + dtz=200, + screening=screening, + wedge_omega_deg=5.0 if screening else None, + ) + + +def test_jfjoch_prepends_pgroup_raw_for_a_rotation(status): + """Step into _format_dataset_settings: jfjoch.py:133. + + Two things to watch: ``data_folder`` is chosen at jfjoch.py:152-169 from the + request type and the screening flag, then joined on at jfjoch.py:189. The + ``data/`` component came from the GUI at stage 3; JFJoch adds none for a + plain rotation. + """ + from aare.devices.jfjoch import JFJochWrapper + + settings = JFJochWrapper._format_dataset_settings( + None, rotation_request(file_prefix="data/xtal7_001", screening=False), status + ) + + assert settings.file_prefix == "group1/raw/data/xtal7_001" + + +def test_jfjoch_routes_raster_requests_to_their_own_folder(status): + """The raster branch at jfjoch.py:152, for contrast with rotation. + + Here JFJoch supplies ``raster`` and the GUI supplies nothing, which is why + raster does not suffer the duplication that screening does. + """ + from aarecommon.math.coordinate import Coordinate, SmargonCoordinate + from aarecommon.models.raster_grid import RasterGridRequest + + from aare.devices.jfjoch import JFJochWrapper + + grid = RasterGridRequest( + file_prefix="xtal7_001_raster2d", + exp_time_s=0.1, + smargon_top_left=SmargonCoordinate(), + n_x=3, + n_y=3, + dtz=200, + grid_size_mm=Coordinate(x=0.05, y=0.05), + omega_deg=0.0, + transmission=1.0, + ) + + settings = JFJochWrapper._format_dataset_settings(None, grid, status) + + assert settings.file_prefix == "group1/raw/raster/xtal7_001_raster2d" + + +@pytest.mark.xfail( + strict=True, + reason=( + "docs 6.1: the GUI prepends 'screening/' (rotation_data_collection.py:303) " + "and JFJoch independently selects data_folder '{pgroup}/raw/screening' " + "(jfjoch.py:162), so the scan kind is encoded twice. The GUI's own preview " + "and run-number collision check build the single-level form, so display, " + "collision check and written path disagree." + ), +) +def test_jfjoch_does_not_duplicate_the_screening_folder(status): + """Step in and compare ``data_folder`` against the incoming ``r.file_prefix``. + + Both say "screening". Fixing this means deciding which side owns the scan + kind -- the GUI transform at stage 3, or the JFJoch routing here. + """ + from aare.devices.jfjoch import JFJochWrapper + + settings = JFJochWrapper._format_dataset_settings( + None, rotation_request(file_prefix="screening/xtal7_001", screening=True), status + ) + + assert settings.file_prefix == "group1/raw/screening/xtal7_001" + + +def test_end_to_end_gui_rotation_name_reaches_the_detector(panel, status): + """Stages 1-3 and 5 joined up, with no mirrored values. + + The only hand-written string is the template a user would type. Step through + this one to watch a name cross every boundary in the path. + """ + from aare.devices.jfjoch import JFJochWrapper + from aare.gui.panels.rotation_data_collection import add_data_to_path + + panel._formatted_date = "20260922" + panel._sample_name = "xtal7" + panel._puck_name = "PUCK1" + panel._puck_pos = 3 + panel.directory_edit.setText("{date}/{puck}/{position}") + panel.file_prefix_edit.setText("{sample}") + + panel.update_filename() # stages 1-2 + request = rotation_request( # stage 3 + file_prefix=str(add_data_to_path(panel._filename)), screening=False + ) + settings = JFJochWrapper._format_dataset_settings(None, request, status) # stage 5 + + assert panel._filename == "20260922/PUCK1/03/xtal7_001" + assert request.file_prefix == "data/20260922/PUCK1/03/xtal7_001" + assert settings.file_prefix == "group1/raw/data/20260922/PUCK1/03/xtal7_001" + + +# --------------------------------------------------------------------------- +# Stage 6 -- what gets recorded in aaredb (aaredb.py:195) +# --------------------------------------------------------------------------- + + +def test_aaredb_records_the_prejfjoch_prefix(status): + """docs 6.5: ``Datasets.filepath = r.file_prefix`` stores the request value. + + That is the value *before* JFJoch prepends ``data_folder``, so the database + path is not the path the file was written to. This test contrasts the two + side by side; there is no seam to call aaredb.py:195 directly without a + database, so the assertion is on the gap itself. + """ + from aare.devices.jfjoch import JFJochWrapper + + request = rotation_request(file_prefix="data/xtal7_001", screening=False) + settings = JFJochWrapper._format_dataset_settings(None, request, status) + + recorded_in_db = request.file_prefix # aaredb.py:195 + written_to_disk = settings.file_prefix + + assert recorded_in_db == "data/xtal7_001" + assert written_to_disk.endswith(recorded_in_db) + assert written_to_disk != recorded_in_db # missing "group1/raw" and the storage root