From 7d997272ef82ddd186d60aa8f32332f88c989b3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=90=D0=BD=D1=82=D0=BE=D0=BD=20=D0=A2=D1=80=D0=BE=D1=88?= =?UTF-8?q?=D0=B8=D0=BD?= Date: Tue, 6 Oct 2026 21:14:07 +0300 Subject: [PATCH] v3.22.3: the mismatch on_fail, the self-healing move, the pruning reconciliation Three test-management issues: - an expectation whose command type did not match the pending command hard-coded level 'error', so on_fail "warning" tolerated "no command pending" but not "a different command is pending". The check now honours the step's on_fail - the tolerant form for "the applet either asks X or does Y" (the mismatched command is still fetched and answered). - moving a script between suites could answer 500 and leave two owners: _test_script_move detached only from the suite named by the script's suite_id and appended to the target unconditionally, so a duplicate listing or a stale suite_id (a hand edit, an interrupted move) collided on the attach. It now detaches from every suite that lists the script, takes the on_fail from the explicit owner (suite_id if that suite lists it, else the first lister), updates the script first (a refused validation changes nothing) and re-attaches with a prune; the same-suite no-op only applies when the target really is the only lister. - reconcile_scripts_to_suites() now prunes duplicate listings to the explicit owner (it only adopted before), so those states self-heal at startup and after a bundle import; the "Imported scripts" suite is only created when there are orphans. Docs: the bundle-import note (importing into the source store duplicates the suite and its scripts - the round-trip is not idempotent) and the expectation on_fail clarification. Tests: 839 frontend + 818 Python; sw cache simple-v407. --- docs/api.md | 15 +++- docs/test-scripts.md | 13 ++- frontend/index.html | 2 +- frontend/sw.js | 2 +- pyproject.toml | 2 +- pysim_simple_server/server.py | 145 +++++++++++++++++++++++----------- tests/test_fetch_routes.py | 57 +++++++++++++ tests/test_test_suites.py | 44 +++++++++++ tests/test_testscript.py | 33 ++++++++ 9 files changed, 258 insertions(+), 55 deletions(-) diff --git a/docs/api.md b/docs/api.md index 32e4487..832734b 100644 --- a/docs/api.md +++ b/docs/api.md @@ -864,10 +864,17 @@ collision gets a fresh uuid and the suites' references are remapped; then the suites are imported (`replace` wipes the suite store first); an unresolvable reference is reported in `errors` and the suite is skipped. After the import the ownership invariant is reconciled: a script the imported suites list adopts -its listing suite, and a script no suite lists any more — the `replace` mode -wipes the suites that owned them — is **adopted into "Imported scripts"** -instead of becoming invisible (the same reconciliation runs at startup for a -v3.21.0 store). +its listing suite, a script listed by several suites keeps its explicit owner +and the other listings are pruned, and a script no suite lists any more — the +`replace` mode wipes the suites that owned them — is **adopted into "Imported +scripts"** instead of becoming invisible (the same reconciliation runs at +startup for a v3.21.0 store). + +The import always **merges**: an id already in the store gets a fresh uuid +(scripts) / a fresh suite id, so importing a bundle **into the store it came +from duplicates the suite and its scripts** — the round-trip is not +idempotent. Restore a bundle into a fresh store, or wipe the target first +(`mode: "replace"` for the suites). ### `POST /api/test/run` (single script or suite) diff --git a/docs/test-scripts.md b/docs/test-scripts.md index 9194ae0..00b6ef4 100644 --- a/docs/test-scripts.md +++ b/docs/test-scripts.md @@ -28,7 +28,10 @@ suite run's report (the PWA's report view, exportable as JSON or Markdown) carries every member's step table, failing notes and run-log excerpt, plus the summary (pass/warn/fail/skipped, wall time, the SCP80 counters before/after per keyset and the card-session flag). The suites live in -`~/.pysim-simple-server/test_suites.json` (`--test-suites`). +`~/.pysim-simple-server/test_suites.json` (`--test-suites`). A bundle +import always merges: importing a suite export **into the store it came +from duplicates the suite and its scripts** (an id already present gets a +fresh uuid) - restore into a fresh store, or wipe the target first. **Format**: server ≥ 3.15.0 (`kvn`, `por` and `files` checks), scripts stored server-side since 3.16.0, `menu-select` by text and the spec-order TERMINAL @@ -385,9 +388,11 @@ runner never polls by itself. `command` is a name (as shown in the proactive log) or a hex type; `ANY`/`*` skips the type check. `qualifier` is the command-details byte 3 (one byte, exact/mask). -When nothing is pending the step fails at its own `on_fail` level (the -default stops the run; `on_fail: "warning"` warns and continues - the -tolerant consume-if-pending form, v3.21.0). "Add a `status` action" applies +When nothing is pending - or **a different command** is pending - the step +fails at its own `on_fail` level (the default stops the run; +`on_fail: "warning"` warns and continues: the tolerant consume-if-pending +form, and the way to express "the applet either asks X or does Y" - the +mismatched command is still fetched and answered with the step's `respond`). "Add a `status` action" applies when the card delivers on poll: `status` with `attempts > 1` polls until a command appears; it cannot make one appear when the card has none. diff --git a/frontend/index.html b/frontend/index.html index ebab15b..f6e0461 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -1903,7 +1903,7 @@ // ===== Version ===== // Single source of truth for the PWA version: shown in the header and used // by the server version check in pysimConnect(). -const SIMPLE_VERSION = '3.22.2'; +const SIMPLE_VERSION = '3.22.3'; document.getElementById('app-version').textContent = 'v' + SIMPLE_VERSION; // ===== Tab switching ===== diff --git a/frontend/sw.js b/frontend/sw.js index 2799058..38ee5af 100644 --- a/frontend/sw.js +++ b/frontend/sw.js @@ -1,4 +1,4 @@ -const CACHE = 'simple-v406'; +const CACHE = 'simple-v407'; const URLS = [ 'index.html', 'help.html', diff --git a/pyproject.toml b/pyproject.toml index 659ea39..976b779 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "pysim-simple-server" -version = "3.22.2" +version = "3.22.3" description = "HTTP REST server wrapping pysim for the SIMple PWA" requires-python = ">=3.8" # pysim is a git-only dependency installed explicitly by setup.bat/setup.sh. diff --git a/pysim_simple_server/server.py b/pysim_simple_server/server.py index 68f4d5c..aad940b 100644 --- a/pysim_simple_server/server.py +++ b/pysim_simple_server/server.py @@ -36,7 +36,7 @@ from osmocom.tlv import BER_TLV_IE from osmocom.utils import rpad -VERSION = '3.22.2' +VERSION = '3.22.3' MAX_ENVELOPE_SEGMENTS = 5 # max SMS segments for outgoing C-APDU in ENVELOPE @@ -5987,30 +5987,52 @@ def _test_script_detach(suites, cur): def _test_script_move(store, suites, cur, target_id): - """Move a script to another suite: the source detaches, the target appends - it as a member (keeping the on_fail policy it had). Both stores are - updated in the one request. Returns ``(entry, error, status)``.""" + """Move a script to another suite: the target appends it as a member + (keeping the on_fail policy it had) and **every** suite that lists it + detaches. The script's own ``suite_id`` names the owner when that suite + lists it, otherwise the first lister does. A duplicate listing or a + stale suite_id (a hand edit, an interrupted move) is repaired by the + move instead of answering 500 and leaving two owners (v3.22.3 review). + Both stores are updated in the one request; returns + ``(entry, error, status)``.""" target = suites.get(target_id) if target is None: return None, 'unknown target suite', 400 - if target['id'] == (cur.get('suite_id') or ''): + listings = [s for s in suites.list() + if any(e['script_id'] == cur['id'] for e in s['scripts'])] + if target['id'] == (cur.get('suite_id') or '') and [s['id'] for s in listings] == [target['id']]: + # the exact no-op: the script already belongs to the target and only + # the target lists it return cur, None, 200 - source = suites.get(cur.get('suite_id')) if cur.get('suite_id') else None + owner = next((s for s in listings if s['id'] == (cur.get('suite_id') or '')), None) + if owner is None and listings: + owner = listings[0] on_fail = 'stop' - if source is not None: - entry = next((e for e in source['scripts'] if e['script_id'] == cur['id']), None) + if owner is not None: + entry = next((e for e in owner['scripts'] if e['script_id'] == cur['id']), None) if entry is not None: on_fail = entry['on_fail'] - _test_script_detach(suites, cur) + # The script first: its validation refuses before any listing changes, so + # a failed move never leaves the script unlisted. try: new = store.update(cur['id'], {'suite_id': target['id']}) except (test_scripts.TestScriptError, OSError) as e: return None, 'cannot move the script: %s' % e, 500 if new is None: return None, 'unknown test script id', 404 + for s in listings: + entries = [e for e in s['scripts'] if e['script_id'] != cur['id']] + if entries == s['scripts']: + continue + try: + suites.update(s['id'], {'scripts': entries}) + except (test_suites.TestSuiteError, OSError) as e: + sys.stderr.write('TESTSUITES: cannot detach %s from "%s": %s\n' + % (cur['id'], s['name'], e)) try: - entries = list(target['scripts']) + [ - {'script_id': cur['id'], 'role': 'member', 'on_fail': on_fail}] + fresh = suites.get(target['id']) or target + entries = [e for e in fresh['scripts'] if e['script_id'] != cur['id']] + entries.append({'script_id': cur['id'], 'role': 'member', 'on_fail': on_fail}) suites.update(target['id'], {'scripts': entries}) except (test_suites.TestSuiteError, OSError) as e: return None, 'cannot attach to the target suite: %s' % e, 500 @@ -6021,6 +6043,10 @@ def reconcile_scripts_to_suites(script_store, suite_store): """Make the ownership invariant hold again: every script belongs to exactly one suite and that suite lists it. + - a script **listed by several suites** (a hand edit or an interrupted + move) stays with its explicit owner - the suite its ``suite_id`` names + when that suite lists it, else the first lister - and the other + listings are pruned; - a script no suite lists (a v3.21.0 store, a suites import with ``mode: "replace"``, a hand edit or an interrupted move) is attached to the auto-created "Imported scripts" suite; @@ -6032,6 +6058,31 @@ def reconcile_scripts_to_suites(script_store, suite_store): if script_store is None or suite_store is None: return 0 suites = suite_store.list() + listings = {} + for s in suites: + for e in s['scripts']: + listings.setdefault(e['script_id'], []).append(s['id']) + by_id = {s['id']: s for s in script_store.list()} + pruned = 0 + for sid, owners in listings.items(): + if len(owners) < 2: + continue + explicit = (by_id.get(sid) or {}).get('suite_id') or None + keep = explicit if explicit in owners else owners[0] + for s in suites: + if s['id'] == keep or s['id'] not in owners: + continue + entries = [e for e in s['scripts'] if e['script_id'] != sid] + try: + suite_store.update(s['id'], {'scripts': entries}) + s['scripts'] = entries + pruned += 1 + sys.stderr.write('TESTSUITES: pruned %s from "%s" (owner "%s")\n' + % (sid, s['name'], keep)) + except (test_suites.TestSuiteError, OSError) as e: + sys.stderr.write('TESTSUITES: cannot prune %s from "%s": %s\n' + % (sid, s['name'], e)) + # The owner map after the pruning (the member list is authoritative). owner = {} for s in suites: for e in s['scripts']: @@ -6053,40 +6104,43 @@ def reconcile_scripts_to_suites(script_store, suite_store): % (script['id'], e)) continue orphans.append(script) - if not orphans and not adopted: + if not orphans and not adopted and not pruned: return 0 - suite = suite_store.find_by_name('Imported scripts') - if suite is None: - suite = suite_store.add({'name': 'Imported scripts'}) - entries = list(suite['scripts']) - known = {e['script_id'] for e in entries} attached = 0 - for s in orphans: - try: - script_store.update(s['id'], {'suite_id': suite['id']}) - except (test_scripts.TestScriptError, OSError) as e: - # The member list is authoritative: list the script anyway. The - # store keeps a script that no longer validates (a tightened rule - # must not destroy it) and its suite_id write is refused by the - # same validator - without the listing the script would be - # invisible in the PWA and could never be fixed in the editor. - sys.stderr.write('TESTSUITES: cannot set the suite id of %s ' - '(listed in "%s" so the editor can fix it): %s\n' - % (s['id'], suite['name'], e)) - if s['id'] not in known: - entries.append({'script_id': s['id'], 'role': 'member', 'on_fail': 'stop'}) - known.add(s['id']) - attached += 1 - if attached: - try: - suite_store.update(suite['id'], {'scripts': entries}) - except (test_suites.TestSuiteError, OSError) as e: - sys.stderr.write('TESTSUITES: cannot fill "%s": %s\n' % (suite['name'], e)) - if attached or adopted: - sys.stderr.write('TESTSUITES: reconciled %d script(s) (attached to "%s": %d, ' - 'adopted by their suite: %d)\n' - % (attached + adopted, suite['name'], attached, adopted)) - return attached + adopted + if orphans: + suite = suite_store.find_by_name('Imported scripts') + if suite is None: + suite = suite_store.add({'name': 'Imported scripts'}) + entries = list(suite['scripts']) + known = {e['script_id'] for e in entries} + for s in orphans: + try: + script_store.update(s['id'], {'suite_id': suite['id']}) + except (test_scripts.TestScriptError, OSError) as e: + # The member list is authoritative: list the script anyway. + # The store keeps a script that no longer validates (a + # tightened rule must not destroy it) and its suite_id write + # is refused by the same validator - without the listing the + # script would be invisible in the PWA and could never be + # fixed in the editor. + sys.stderr.write('TESTSUITES: cannot set the suite id of %s ' + '(listed in "%s" so the editor can fix it): %s\n' + % (s['id'], suite['name'], e)) + if s['id'] not in known: + entries.append({'script_id': s['id'], 'role': 'member', 'on_fail': 'stop'}) + known.add(s['id']) + attached += 1 + if attached: + try: + suite_store.update(suite['id'], {'scripts': entries}) + except (test_suites.TestSuiteError, OSError) as e: + sys.stderr.write('TESTSUITES: cannot fill "%s": %s\n' % (suite['name'], e)) + if attached or adopted or pruned: + sys.stderr.write('TESTSUITES: reconciled %d script(s) (attached to ' + '"Imported scripts": %d, adopted by their suite: %d, ' + 'pruned duplicate listings: %d)\n' + % (attached + adopted + pruned, attached, adopted, pruned)) + return attached + adopted + pruned def migrate_scripts_to_suites(script_store, suite_store): @@ -6849,9 +6903,12 @@ def _test_run_expect(server, step, pending, ctx=None): checks.append(_test_check_result('Command', True, step['command'].get('name') or type_name, '%s (0x%02X)' % (type_name, cmd_type), 'ok')) else: + # A different command pending is a check failure like any other: it + # honours the step's on_fail, so on_fail "warning" expresses "the + # applet either asks X or Y" (the command is still fetched/answered). checks.append(_test_check_result( 'Command', False, step['command'].get('name') or '0x%02X' % want_type, - '%s (0x%02X)' % (type_name, cmd_type), 'error')) + '%s (0x%02X)' % (type_name, cmd_type), step['on_fail'])) if step.get('qualifier'): actual_q = '%02X' % cmd_qual if cmd_qual is not None else None checks.append(_test_check_result('Qualifier', diff --git a/tests/test_fetch_routes.py b/tests/test_fetch_routes.py index dcbe7cc..73bbb9e 100644 --- a/tests/test_fetch_routes.py +++ b/tests/test_fetch_routes.py @@ -1360,6 +1360,63 @@ class TestScriptStoreHttpTests(unittest.TestCase): self.assertEqual(status, 400, resp) self.assertIn('member set changes', resp['error']) + def test_move_repairs_a_duplicate_listing(self): + # v3.22.3: the move detaches the script from EVERY suite that lists + # it and re-attaches to the target - an inconsistent state (a partial + # move, a stale suite_id) must not answer 500 or leave two owners. + a = self._suite(name='a') + b = self._suite(name='b') + status, resp = self._post('/api/test/scripts', self._script(suite_id=a['id'])) + self.assertEqual(status, 200, resp) + sid = resp['script']['id'] + # b lists it too: the state a partial move leaves behind + self.suites.update(b['id'], {'scripts': [ + {'script_id': sid, 'role': 'member', 'on_fail': 'continue'}]}) + status, resp = self._post('/api/test/scripts/update', { + 'id': sid, 'script': {'name': 'demo', 'suite_id': b['id']}}) + self.assertEqual(status, 200, resp) + self.assertTrue(resp.get('moved')) + self.assertEqual(self.suites.get(a['id'])['scripts'], []) + self.assertEqual([e['script_id'] for e in self.suites.get(b['id'])['scripts']], + [sid]) + self.assertEqual(self.store.get(sid)['suite_id'], b['id']) + # the on_fail comes from the explicit owner (the suite_id named a) + self.assertEqual(self.suites.get(b['id'])['scripts'][0]['on_fail'], 'stop') + + def test_move_from_a_stale_owner_repairs_the_listing(self): + a = self._suite(name='a') + b = self._suite(name='b') + status, resp = self._post('/api/test/scripts', self._script(suite_id=a['id'])) + self.assertEqual(status, 200, resp) + sid = resp['script']['id'] + # the suite_id points at a missing suite while a still lists it + self.store.update(sid, {'suite_id': 'f' * 32}) + status, resp = self._post('/api/test/scripts/update', { + 'id': sid, 'script': {'name': 'demo', 'suite_id': b['id']}}) + self.assertEqual(status, 200, resp) + self.assertEqual(self.suites.get(a['id'])['scripts'], []) + self.assertEqual([e['script_id'] for e in self.suites.get(b['id'])['scripts']], + [sid]) + self.assertEqual(self.store.get(sid)['suite_id'], b['id']) + + def test_move_to_the_same_suite_prunes_a_duplicate(self): + a = self._suite(name='a') + b = self._suite(name='b') + status, resp = self._post('/api/test/scripts', self._script(suite_id=b['id'])) + self.assertEqual(status, 200, resp) + sid = resp['script']['id'] + # a also lists it; the explicit suite_id names b, whose on_fail stays + self.suites.update(a['id'], {'scripts': [ + {'script_id': sid, 'role': 'member', 'on_fail': 'stop'}]}) + self.suites.update(b['id'], {'scripts': [ + {'script_id': sid, 'role': 'member', 'on_fail': 'continue'}]}) + status, resp = self._post('/api/test/scripts/move', {'id': sid, 'suite_id': b['id']}) + self.assertEqual(status, 200, resp) + self.assertEqual(self.suites.get(a['id'])['scripts'], []) + self.assertEqual([e['script_id'] for e in self.suites.get(b['id'])['scripts']], + [sid]) + self.assertEqual(self.suites.get(b['id'])['scripts'][0]['on_fail'], 'continue') + def test_move_and_copy_between_suites(self): a = self._suite(name='a') b = self._suite(name='b') diff --git a/tests/test_test_suites.py b/tests/test_test_suites.py index 3a2078f..acd0573 100644 --- a/tests/test_test_suites.py +++ b/tests/test_test_suites.py @@ -222,6 +222,50 @@ class MigrationTests(unittest.TestCase): # idempotent: nothing left to attach self.assertEqual(srv.migrate_scripts_to_suites(scripts, suites), 0) + def test_a_duplicate_listing_is_pruned_to_the_explicit_owner(self): + # v3.22.3: the reconciliation keeps a script in the suite its + # suite_id names and prunes the other listings (a hand edit or an + # interrupted move must not leave two owners). + from pysim_simple_server import server as srv + from pysim_simple_server import test_scripts + with tempfile.TemporaryDirectory() as tmp: + scripts = test_scripts.TestScriptStore(pathlib.Path(tmp) / 's.json') + suites = test_suites.TestSuiteStore(pathlib.Path(tmp) / 'u.json') + step = {'type': 'action', 'kind': 'status', 'params': {'attempts': 1}} + a = suites.add({'name': 'A'}) + b = suites.add({'name': 'B'}) + s = scripts.add({'name': 'x', 'suite_id': b['id'], 'steps': [step]}) + for su in (a, b): + suites.update(su['id'], {'scripts': [ + {'script_id': s['id'], 'role': 'member', 'on_fail': 'stop'}]}) + self.assertEqual(srv.reconcile_scripts_to_suites(scripts, suites), 1) + self.assertEqual(suites.get(a['id'])['scripts'], []) + self.assertEqual([e['script_id'] for e in suites.get(b['id'])['scripts']], + [s['id']]) + self.assertEqual(scripts.get(s['id'])['suite_id'], b['id']) + # idempotent + self.assertEqual(srv.reconcile_scripts_to_suites(scripts, suites), 0) + + def test_a_duplicate_listing_with_a_stale_owner_keeps_the_first_lister(self): + from pysim_simple_server import server as srv + from pysim_simple_server import test_scripts + with tempfile.TemporaryDirectory() as tmp: + scripts = test_scripts.TestScriptStore(pathlib.Path(tmp) / 's.json') + suites = test_suites.TestSuiteStore(pathlib.Path(tmp) / 'u.json') + step = {'type': 'action', 'kind': 'status', 'params': {'attempts': 1}} + a = suites.add({'name': 'A'}) + b = suites.add({'name': 'B'}) + s = scripts.add({'name': 'x', 'suite_id': 'f' * 32, 'steps': [step]}) + for su in (a, b): + suites.update(su['id'], {'scripts': [ + {'script_id': s['id'], 'role': 'member', 'on_fail': 'stop'}]}) + # the prune of B plus the adoption of the first lister A + self.assertEqual(srv.reconcile_scripts_to_suites(scripts, suites), 2) + self.assertEqual([e['script_id'] for e in suites.get(a['id'])['scripts']], + [s['id']]) + self.assertEqual(suites.get(b['id'])['scripts'], []) + self.assertEqual(scripts.get(s['id'])['suite_id'], a['id']) + def test_an_invalid_script_is_still_listed_by_the_reconciliation(self): # The script store keeps an entry that no longer validates (a # tightened rule must not destroy it). Its suite_id write is refused diff --git a/tests/test_testscript.py b/tests/test_testscript.py index a597795..4d170cf 100644 --- a/tests/test_testscript.py +++ b/tests/test_testscript.py @@ -563,6 +563,39 @@ class TestRunnerDialogue(RunnerTestCase): self.assertIn('no proactive command pending', run['steps'][0]['checks'][0]['detail']) + def test_a_command_mismatch_honours_on_fail(self): + # v3.22.3: a different command pending is a check failure at the + # step's on_fail level, like every other expectation check - so + # on_fail "warning" expresses "the applet either asks X or does Y" + # (the mismatched command is still fetched and answered). + scc = FakeScc() + scc.push('80F2', '', '9102') # STATUS: DISPLAY TEXT pending + scc.push('8012', DISPLAY_TEXT_CMD, '9000') + scc.push('8014', '', '9000') + run = self.run_script(FakeServer(scc), [ + {'type': 'action', 'kind': 'status', 'params': {'attempts': 2}}, + {'type': 'expect', 'command': 'SEND SHORT MESSAGE', + 'respond': {'result': 'ok'}, 'on_fail': 'warning'}, + ]) + self.assertEqual(run['status'], 'warning', run['steps']) + check = run['steps'][1]['checks'][0] + self.assertEqual(check['label'], 'Command') + self.assertFalse(check['ok']) + self.assertEqual(check['level'], 'warning') + self.assertIn('DISPLAY TEXT', check['actual']) + self.assertTrue(run['steps'][1]['sent'].startswith('TR ')) + # the default (error) still stops the run + scc2 = FakeScc() + scc2.push('80F2', '', '9102') + scc2.push('8012', DISPLAY_TEXT_CMD, '9000') + scc2.push('8014', '', '9000') + run2 = self.run_script(FakeServer(scc2), [ + {'type': 'action', 'kind': 'status', 'params': {'attempts': 2}}, + {'type': 'expect', 'command': 'SEND SHORT MESSAGE', + 'respond': {'result': 'ok'}}, + ]) + self.assertEqual(run2['status'], 'error') + def test_text_mismatch_fails_the_expectation(self): scc = FakeScc() scc.push('80C2', '', '9105')