ECS-6433 heuristic loading #231

janeliu-slac commented 1 month ago

Description

Updated happi's load function to support searching and loading in one user-friendly expression. The load function gathers the results of each search term individually and loads the union of the results.
Cleaned up code by removing stray print statements.
Added unit tests for load function.
In load function of cli.py removed term=[] and replaced with [item] for readability.

Motivation and Context

It allows users to search and load device objects using one CLI expression.

https://github.com/pcdshub/happi/issues/231

How Has This Been Tested?

Manual testing and unit tests.

Where Has This Been Documented?

Documentation is in the code and release notes.

Pre-merge checklist

[x] Code works interactively
[x] Code contains descriptive docstrings, including context and API
[x] New/changed functions and methods are covered in the test suite where possible
[x] Test suite passes locally
[x] Test suite passes on GitHub Actions
[x] Ran docs/pre-release-notes.sh and created a pre-release documentation page
[x] Pre-release docs include context, functional descriptions, and contributors as appropriate

tangkong commented 1 month ago

As an aside, I think it's worthwhile developing the tests as you write the code. I'm often guilty of writing code then making tests after, but practicing proper test-driven development can often help speed up your development.

In this case, if you set up tests that ran the happi load CLI with various argument combinations, you'd have uncovered some of the bugs I mentioned above

janeliu-slac commented 1 month ago

I updated the test_load_glob_args function so for res in results will print out the name of each of the mock devices, but got stuck trying to get those names into runner.invoke(happi_cli, ['--path', happi_cfg, 'load', name1, name2]). Is there a way to add these names without hardcoding it into runner.invoke? Currently, client.search_regex(name='tst_base_.*') generates 2 results, but there may be more if someone adds more mock devices in the future.

def test_load_glob_args(
    caplog: pytest.LogCaptureFixture,
    client: happi.client.Client,
    happi_cfg: str,
    runner: CliRunner
):
    # get search results
    results = client.search_regex(name='tst_base_.*')

    for res in results:
        name = res.item['name']  # the results are 'tst_base_pim' and 'tst_base_pim2'
        print(name)

    # try to load the item
    with mock.patch.object(IPython, 'start_ipython') as m:
        _ = runner.invoke(
            happi_cli, ['--path', happi_cfg, 'load', 'tst_base_pim', 'tst_base_pim2']
        )
        m.assert_called_once_with(argv=['--quick'], user_ns=devices)        
    with caplog.at_level(logging.INFO):
        assert "Creating shell with devices" in caplog.text

ZLLentz commented 1 month ago

Hi Jane, if you want to discuss your code you should push it so we can make in-line comments. The test you show here calls runner.invoke without using the glob so I'm confused

janeliu-slac commented 1 month ago

It seems like client.load_device() isn't able to accept globs so I used client.search_regex(name='tst_base_.*'), which returns multiple SearchResult objects. I was able to get name from SearchResult objects but wasn't sure how to pass them into runner.invoke(). I rewrote the code and just commited it (it's definitely not passing pytest).

tangkong commented 1 month ago

I swear these are my last nitpicks, then I think we're good to go

janeliu-slac commented 1 month ago

Lol it's fine. I'm learning a lot and taking notes for future projects.

pcdshub / happi