From 3471b05eec897073e3642b9ecb42e0f084ec1146 Mon Sep 17 00:00:00 2001 From: tangkong Date: Mon, 18 Sep 2023 12:28:53 -0700 Subject: [PATCH 1/8] BUG: fix happi transfer default handling, add a test transferring to container with more fields --- happi/prompt.py | 7 +++++-- happi/tests/conftest.py | 8 ++++++++ happi/tests/test_cli.py | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/happi/prompt.py b/happi/prompt.py index 1f8ce3cc..f1401d10 100644 --- a/happi/prompt.py +++ b/happi/prompt.py @@ -183,8 +183,10 @@ def transfer_container(client, item, target): f'entry "{nt}"') d = getattr(getattr(target, nt), 'default') val = click.prompt(missing_prompt, default=f'take default: {d}') - if val != 'take default': + if not ('take default' in val): edits.update({nt: val}) + else: + edits.update({nt: d}) # Actually apply changes and cast item into new container # Enforce conditions are dealt with here @@ -198,7 +200,8 @@ def transfer_container(client, item, target): except TransferError as e: print(e) fix_prompt = f'New value for "{e.key}"' - new_val = click.prompt(fix_prompt) + d = getattr(getattr(target, e.key), 'default') + new_val = click.prompt(fix_prompt, default=d) edits.update({e.key: new_val}) if not new_kwargs: diff --git a/happi/tests/conftest.py b/happi/tests/conftest.py index 3ed60a3d..6da24e1a 100644 --- a/happi/tests/conftest.py +++ b/happi/tests/conftest.py @@ -449,6 +449,14 @@ def db(tmp_path): "type": "HappiItem", "z": 6.0, "y": 10.0 + }, + "tst_minimal": { + "_id": "tst_minimal", + "creation": "Tue Jan 29 09:46:00 2019", + "device_class": "types.SimpleNamespace", + "last_edit": "Thu Sep 14 14:40:08 2018", + "name": "tst_minimal", + "type": "HappiItem" } } """) diff --git a/happi/tests/test_cli.py b/happi/tests/test_cli.py index 730018e7..c9926939 100644 --- a/happi/tests/test_cli.py +++ b/happi/tests/test_cli.py @@ -700,6 +700,39 @@ def test_transfer_cli( assert_match_expected(results, expected_output) +@pytest.mark.parametrize("from_user, expected_output", [ + pytest.param('\n'.join(['', '']), [ + 'Attempting to transfer tst_minimal to OphydItem...', + '+---------------+---------------+', + '| tst_minimal | OphydItem |', + '+---------------+---------------+', + '| active | active |', + '| args | args |', + '| device_class | device_class |', + '| documentation | documentation |', + '| kwargs | kwargs |', + '| name | name |', + '| - | prefix |', + '+---------------+---------------+', + '', + '----------Prepare Entries-----------', + 'OphydItem expects information for entry "prefix" [take default: None]: n', + '', + '----------Amend Entries-----------', + 'Save final item? [y/N]: n', + ''], id="transfer_succeeding", + )]) +def test_transfer_cli_more( + from_user: str, + expected_output: list[str], + happi_cfg: str, + runner: CliRunner +): + results = runner.invoke(happi_cli, ['--path', happi_cfg, 'transfer', + 'tst_minimal', 'OphydItem'], input=from_user) + assert_match_expected(results, expected_output) + + def arg_variants(variants: tuple[tuple[tuple[str]]]): """ Collapse argument variants into all possible combinations. From cf8925eafb9d2711356e365095203cc1c3cbd066 Mon Sep 17 00:00:00 2001 From: tangkong Date: Mon, 18 Sep 2023 13:16:23 -0700 Subject: [PATCH 2/8] TST: make conftest.trim_split_output actually return a list of sttrings and fix affected tests --- happi/tests/test_cli.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/happi/tests/test_cli.py b/happi/tests/test_cli.py index c9926939..f27f2d4d 100644 --- a/happi/tests/test_cli.py +++ b/happi/tests/test_cli.py @@ -44,14 +44,14 @@ def trim_split_output(strings: str, delim: str = '\n'): """ date_pattern = r"\[(\d{4})[-](0[1-9]|1[012])[-].*\]" - ok_substrs = [r"happi.item."] + omit_substrs = [r"happi.containers."] # remove registry items - new_out = [ - st for st in strings.split(delim) - if any([re.search(substr, st) for substr in ok_substrs]) - ] + new_out = strings.split(delim) + # strip registry items from outside entrypoints? + new_out = [st for st in new_out + if not any([re.search(substr, st) for substr in omit_substrs])] # strip date-time from logging messages new_out = [re.sub(date_pattern, '', st) for st in new_out] return new_out @@ -252,17 +252,17 @@ def test_search_json(runner: CliRunner, happi_cfg: str): }, "active": true, "documentation": null, - "prefix": "TST:BASE:PIM2", - "_id": "TST_BASE_PIM2", + "_id": "tst_base_pim2", "beamline": "TST", "creation": "Wed Jan 30 09:46:00 2019", "last_edit": "Fri Apr 13 14:40:08 2018", "macros": null, "parent": null, + "prefix": "TST:BASE:PIM2", "screen": null, "stand": "BAS", "system": "diagnostic", - "type": "OphydItem", + "type": "HappiItem", "z": 6.0, "y": 10.0 } @@ -716,10 +716,10 @@ def test_transfer_cli( '+---------------+---------------+', '', '----------Prepare Entries-----------', - 'OphydItem expects information for entry "prefix" [take default: None]: n', + 'OphydItem expects information for entry "prefix" [take default: None]: ', '', '----------Amend Entries-----------', - 'Save final item? [y/N]: n', + 'Save final item? [y/N]: ', ''], id="transfer_succeeding", )]) def test_transfer_cli_more( From 855ea22c7fabe9ca61da2019a94ccdd4f123535d Mon Sep 17 00:00:00 2001 From: tangkong Date: Mon, 18 Sep 2023 13:18:19 -0700 Subject: [PATCH 3/8] DOC: pre-release notes --- .../335-bug_xfer_default.rst | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 docs/source/upcoming_release_notes/335-bug_xfer_default.rst diff --git a/docs/source/upcoming_release_notes/335-bug_xfer_default.rst b/docs/source/upcoming_release_notes/335-bug_xfer_default.rst new file mode 100644 index 00000000..ca2331d8 --- /dev/null +++ b/docs/source/upcoming_release_notes/335-bug_xfer_default.rst @@ -0,0 +1,23 @@ +335 bug_xfer_default +#################### + +API Changes +----------- +- N/A + +Features +-------- +- N/A + +Bugfixes +-------- +- Fixes bug where `happi transfer` was not filling default values properly +- Fixes conftest.trim_split_output, which was effectively a no-op. Touches up affected tests + +Maintenance +----------- +- N/A + +Contributors +------------ +- tangkong From 9eb2f052514cccc84bc27dacc89fab5c5f735d0e Mon Sep 17 00:00:00 2001 From: tangkong Date: Mon, 18 Sep 2023 13:40:38 -0700 Subject: [PATCH 4/8] TST: also omit DEBUG logging messages --- happi/tests/test_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/happi/tests/test_cli.py b/happi/tests/test_cli.py index f27f2d4d..720df512 100644 --- a/happi/tests/test_cli.py +++ b/happi/tests/test_cli.py @@ -44,7 +44,7 @@ def trim_split_output(strings: str, delim: str = '\n'): """ date_pattern = r"\[(\d{4})[-](0[1-9]|1[012])[-].*\]" - omit_substrs = [r"happi.containers."] + omit_substrs = [r"happi.containers.", r"- DEBUG -"] # remove registry items new_out = strings.split(delim) From 33cafaa57985e333274bbc3f07b4846cf089700c Mon Sep 17 00:00:00 2001 From: tangkong Date: Mon, 18 Sep 2023 14:50:38 -0700 Subject: [PATCH 5/8] TST: Disable checking against stdout by default, instead simply printing if mismatched. Make sure tests actually check effects of command --- happi/tests/test_cli.py | 52 ++++++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/happi/tests/test_cli.py b/happi/tests/test_cli.py index 720df512..24957309 100644 --- a/happi/tests/test_cli.py +++ b/happi/tests/test_cli.py @@ -60,7 +60,8 @@ def trim_split_output(strings: str, delim: str = '\n'): def assert_match_expected( result: click.testing.Result, expected_output: Iterable[str], - expected_return: int = 0 + expected_return: int = 0, + raise_failures: bool = False ): """ standard checks for a cli result, confirms output matches expected @@ -68,11 +69,17 @@ def assert_match_expected( """ logger.debug(result.output) assert result.exit_code == expected_return - assert not result.exception or (isinstance(result.exception, SystemExit) and expected_return != 0) + assert not result.exception or (isinstance(result.exception, SystemExit) + and expected_return != 0) trimmed_output = trim_split_output(result.output) for message, expected in zip(trimmed_output, expected_output): - assert message == expected + if raise_failures: + assert message == expected + else: + if not message == expected: + print('Output does not match expected value:\n' + f'- Output: ({message}\n- Expected: {expected}') def assert_in_expected( @@ -273,7 +280,7 @@ def test_search_json(runner: CliRunner, happi_cfg: str): ) assert result.exit_code == 0 - assert_match_expected(result, expected_output.split('\n')) + assert_match_expected(result, expected_output.split('\n'), raise_failures=True) @pytest.mark.parametrize("from_user, expected_output, expected_return", [ @@ -390,6 +397,13 @@ def test_add_cli( result = runner.invoke(happi_cli, ['--path', happi_cfg, 'add'], input=from_user) assert_match_expected(result, expected_output, expected_return) + # check this has been added + dev_name = from_user[1] + if '_' not in dev_name: # special case for errored + return + client = happi.client.Client.from_config(cfg=happi_cfg) + item = client.find_item(name=dev_name) + assert item["_id"] def test_delete_cli( @@ -464,6 +478,10 @@ def test_add_clone( input=from_user) assert_match_expected(clone_result, expected_output) + client = happi.client.Client.from_config(cfg=happi_cfg) + item = client.find_item(name="happi_new_name") + assert item["type"] == 'HappiItem' + assert item["device_class"] == 'device_class' def test_add_clone_item_not_found(happi_cfg: str, runner: CliRunner): @@ -656,7 +674,7 @@ def test_update_stdin(wrapper: str, happi_cfg: str, runner: CliRunner): @pytest.mark.parametrize("from_user, expected_output", [ - pytest.param('\n'.join(['n', 'n', 'n', 'n', 'N', 'n', 'n', 'n', '']), [ + pytest.param('\n'.join(['n', 'n', 'n', 'n', 'N', 'n', 'n', 'n', 'y']), [ 'Attempting to transfer tst_base_pim to OphydItem...', '+---------------+---------------+', '| tst_base_pim | OphydItem |', @@ -686,7 +704,11 @@ def test_update_stdin(wrapper: str, happi_cfg: str, runner: CliRunner): 'Include entry from tst_base_pim: system = "diagnostic"? [Y/n]: n', 'Include entry from tst_base_pim: y = "40.0"? [Y/n]: n', 'Include entry from tst_base_pim: z = "3.0"? [Y/n]: n', '', - '----------Amend Entries-----------', 'Save final item? [y/N]: ', + '----------Amend Entries-----------', 'Save final item? [y/N]: y', + ' - INFO - Attempting to remove OphydItem (name=tst_base_pim) from the collection ...', + ' - INFO - Storing item OphydItem (name=tst_base_pim) ...', + ' - INFO - Adding / Modifying information for tst_base_pim ...', + ' - INFO - HappiItem OphydItem (name=tst_base_pim) has been succesfully added to the database', ''], id="transfer_succeeding", )]) def test_transfer_cli( @@ -698,10 +720,14 @@ def test_transfer_cli( results = runner.invoke(happi_cli, ['--path', happi_cfg, 'transfer', 'tst_base_pim', 'OphydItem'], input=from_user) assert_match_expected(results, expected_output) + client = happi.client.Client.from_config(cfg=happi_cfg) + item = client.find_item(name="tst_base_pim") + assert item["type"] == 'OphydItem' + assert getattr(item, 'parent', 'DNE') == 'DNE' @pytest.mark.parametrize("from_user, expected_output", [ - pytest.param('\n'.join(['', '']), [ + pytest.param('\n'.join(['MY:PREFIX', 'y']), [ 'Attempting to transfer tst_minimal to OphydItem...', '+---------------+---------------+', '| tst_minimal | OphydItem |', @@ -716,10 +742,14 @@ def test_transfer_cli( '+---------------+---------------+', '', '----------Prepare Entries-----------', - 'OphydItem expects information for entry "prefix" [take default: None]: ', + 'OphydItem expects information for entry "prefix" [take default: None]: MY:PREFIX', '', '----------Amend Entries-----------', - 'Save final item? [y/N]: ', + 'Save final item? [y/N]: y', + ' - INFO - Attempting to remove OphydItem (name=tst_minimal) from the collection ...', + ' - INFO - Storing item OphydItem (name=tst_minimal) ...', + ' - INFO - Adding / Modifying information for tst_minimal ...', + ' - INFO - HappiItem OphydItem (name=tst_minimal) has been succesfully added to the database', ''], id="transfer_succeeding", )]) def test_transfer_cli_more( @@ -731,6 +761,10 @@ def test_transfer_cli_more( results = runner.invoke(happi_cli, ['--path', happi_cfg, 'transfer', 'tst_minimal', 'OphydItem'], input=from_user) assert_match_expected(results, expected_output) + client = happi.client.Client.from_config(cfg=happi_cfg) + item = client.find_item(name="tst_minimal") + assert item["type"] == 'OphydItem' + assert item["prefix"] == 'MY:PREFIX' def arg_variants(variants: tuple[tuple[tuple[str]]]): From 31867acc019ac11f7b7c1a7a2f87ad4519e0b578 Mon Sep 17 00:00:00 2001 From: tangkong Date: Mon, 18 Sep 2023 14:55:54 -0700 Subject: [PATCH 6/8] DOC: update pre-release notes --- docs/source/upcoming_release_notes/335-bug_xfer_default.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/source/upcoming_release_notes/335-bug_xfer_default.rst b/docs/source/upcoming_release_notes/335-bug_xfer_default.rst index ca2331d8..7c641113 100644 --- a/docs/source/upcoming_release_notes/335-bug_xfer_default.rst +++ b/docs/source/upcoming_release_notes/335-bug_xfer_default.rst @@ -16,7 +16,9 @@ Bugfixes Maintenance ----------- -- N/A +- Tests modified to no longer assert stdout matches expected strings. Rather the effect of the + command being tested is verified independently. The `assert_match_expected` helper is still + used, but will now print mismatches instead of asserting them. Contributors ------------ From 781b481a6b01ccfc0df39b98681b3a0f7627c910 Mon Sep 17 00:00:00 2001 From: Robert Tang-Kong <35379409+tangkong@users.noreply.github.com> Date: Mon, 18 Sep 2023 16:08:36 -0700 Subject: [PATCH 7/8] MNT: better getattr access Co-authored-by: Ken Lauer --- happi/prompt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/happi/prompt.py b/happi/prompt.py index f1401d10..9270e5e5 100644 --- a/happi/prompt.py +++ b/happi/prompt.py @@ -200,7 +200,7 @@ def transfer_container(client, item, target): except TransferError as e: print(e) fix_prompt = f'New value for "{e.key}"' - d = getattr(getattr(target, e.key), 'default') + d = getattr(target, e.key).default new_val = click.prompt(fix_prompt, default=d) edits.update({e.key: new_val}) From e1bc7b83fa6453b7bd5799501f4b8744e26447b5 Mon Sep 17 00:00:00 2001 From: tangkong Date: Mon, 18 Sep 2023 17:08:48 -0700 Subject: [PATCH 8/8] MNT: use prompt_for_entry in transfer_container --- happi/prompt.py | 17 ++++++----------- happi/tests/test_cli.py | 4 ++-- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/happi/prompt.py b/happi/prompt.py index 9270e5e5..5748e676 100644 --- a/happi/prompt.py +++ b/happi/prompt.py @@ -6,6 +6,7 @@ import prettytable from happi.errors import EnforceError, TransferError +from happi.item import EntryInfo from happi.utils import (OptionalDefault, is_valid_identifier_not_keyword, optional_enforce) @@ -179,14 +180,9 @@ def transfer_container(client, item, target): # Deal with missing keys in target, deal with validation later for nt in target_exclusive: - missing_prompt = (f'{target_name} expects information for ' - f'entry "{nt}"') - d = getattr(getattr(target, nt), 'default') - val = click.prompt(missing_prompt, default=f'take default: {d}') - if not ('take default' in val): - edits.update({nt: val}) - else: - edits.update({nt: d}) + einfo: EntryInfo = getattr(target, nt) + val = prompt_for_entry(einfo) + edits.update({nt: val}) # Actually apply changes and cast item into new container # Enforce conditions are dealt with here @@ -199,9 +195,8 @@ def transfer_container(client, item, target): success = True except TransferError as e: print(e) - fix_prompt = f'New value for "{e.key}"' - d = getattr(target, e.key).default - new_val = click.prompt(fix_prompt, default=d) + einfo = getattr(target, e.key) + new_val = prompt_for_entry(einfo) edits.update({e.key: new_val}) if not new_kwargs: diff --git a/happi/tests/test_cli.py b/happi/tests/test_cli.py index 24957309..1f72946d 100644 --- a/happi/tests/test_cli.py +++ b/happi/tests/test_cli.py @@ -79,7 +79,7 @@ def assert_match_expected( else: if not message == expected: print('Output does not match expected value:\n' - f'- Output: ({message}\n- Expected: {expected}') + f'- Output: {message}\n- Expected: {expected}') def assert_in_expected( @@ -742,7 +742,7 @@ def test_transfer_cli( '+---------------+---------------+', '', '----------Prepare Entries-----------', - 'OphydItem expects information for entry "prefix" [take default: None]: MY:PREFIX', + 'Enter value for prefix, enforce=str: MY:PREFIX', '', '----------Amend Entries-----------', 'Save final item? [y/N]: y',