Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

service: Add generic get_connection(s) API #438

Merged
merged 31 commits into from
Oct 12, 2023
Merged

service: Add generic get_connection(s) API #438

merged 31 commits into from
Oct 12, 2023

Conversation

bkeryan
Copy link
Collaborator

@bkeryan bkeryan commented Oct 10, 2023 •
edited by dixonjoel
Loading

What does this Pull Request accomplish?

Add reservation.get_connection and reservation.get_connections for mapping a pin and site to a specific instrument session and channel.

Update the reservation classes to eagerly convert the gRPC session infos to API session infos.

Why should this Pull Request be merged?

Part of the new session management API.

What testing has been done?

Ran new unit tests.

@bkeryan bkeryan requested a review from jasonmreding October 10, 2023 00:19
@github-actions
Copy link

github-actions bot commented Oct 10, 2023 •
edited
Loading

Test Results

       30 files  ±       0       30 suites  ±0   14m 25s ⏱️ + 3m 33s
     381 tests +     43     305 ✔️ +     34       76 💤 +    9  0 ❌ ±0 
11 400 runs  +1 290  8 585 ✔️ +1 020  2 815 💤 +270  0 ❌ ±0 

Results for commit 2d7e530. ± Comparison against base commit 49a90c6.

This pull request removes 4 and adds 47 tests. Note that renamed tests count towards both. 
tests.unit.test_reservation ‑ test___multi_session_infos___create_session___value_error_raised
tests.unit.test_reservation ‑ test___multi_session_infos___create_sessions___session_infos_yielded
tests.unit.test_reservation ‑ test___multi_session_infos___create_sessions___session_lifetime_tracked
tests.unit.test_reservation ‑ test___multi_session_infos___create_sessions___sessions_created

tests.integration.test_reservation ‑ test___sessions_reserved___get_connections___connections_returned
tests.integration.test_reservation ‑ test___sessions_reserved___get_connections_by_instrument_type___connections_returned
tests.integration.test_reservation ‑ test___sessions_reserved___get_connections_by_pin___connections_returned
tests.integration.test_reservation ‑ test___sessions_reserved___get_connections_by_site___connections_returned
tests.integration.test_reservation ‑ test___sessions_reserved_with_relays___get_connections_for_relay_driver___connections_returned
tests.integration.test_reservation ‑ test___sessions_reserved_with_relays___get_connections_for_relay_driver_by_site___connections_returned
tests.integration.test_reservation ‑ test___sessions_reserved_with_shared_pins_all_sites___get_connections___connections_returned_for_first_matching_site
tests.integration.test_reservation ‑ test___sessions_reserved_with_shared_pins_site0___get_connections___connections_returned
tests.integration.test_reservation ‑ test___sessions_reserved_with_shared_pins_site1___get_connections___connections_returned
tests.unit.test_reservation ‑ test___heterogenous_session_infos___get_connection_with_instrument_type_id___connection_returned[nibar-Pin4-5-6]
…

♻️ This comment has been updated with latest results.

@bkeryan bkeryan requested a review from pbirkhol-ni October 11, 2023 22:19
@bkeryan
service: Remove unnecessary repr() usage
2d7e530 
For strings, repr() doesn't just add single quotes. It also escapes
special characters.
mshafer-NI
mshafer-NI approved these changes Oct 12, 2023
View reviewed changes
Copy link
Collaborator

@mshafer-NI mshafer-NI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments

@bkeryan bkeryan merged commit 9dbb470 into main Oct 12, 2023
@bkeryan bkeryan deleted the users/bkeryan/get-connections branch October 12, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mshafer-NI mshafer-NI mshafer-NI approved these changes

@dixonjoel dixonjoel dixonjoel approved these changes

@pbirkhol-ni pbirkhol-ni pbirkhol-ni approved these changes

@csjall csjall Awaiting requested review from csjall csjall is a code owner

@nstokes-nati nstokes-nati Awaiting requested review from nstokes-nati nstokes-nati is a code owner

@jasonmreding jasonmreding Awaiting requested review from jasonmreding

@jonathanmendez jonathanmendez Awaiting requested review from jonathanmendez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bkeryan @mshafer-NI @dixonjoel @pbirkhol-ni @jasonmreding