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.

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

service: Update reserve_session(s) to accept a single pin and explicitly specify default timeout #389

Merged
merged 11 commits into from
Sep 21, 2023

Conversation

bkeryan
Copy link
Collaborator

@bkeryan bkeryan commented Sep 20, 2023

What does this Pull Request accomplish?

SessionManagementClient:

  • Update reserve_session(s) to accept a single pin for the pin_or_relay_names argument.
  • Change reserve_session(s) and reserve_all_registered_sessions to explicitly specify a default timeout of 0.0.
  • Change timeout handling to raise a RuntimeWarning when you specify a negative timeout other than -1, such as timeout=-2 or timeout=-1e6. This maintains compatibility with the existing behavior, but encourages clients to use -1 for an infinite timeout.
  • Update documentation.
  • Add more unit test cases.

Why should this Pull Request be merged?

Accepting a single pin allows calling reserve_session(context, "Vdd") without wrapping the pin name in a list or tuple.

In Python, str is a Iterable[str] that iterates over the characters of the string. Before this change, reserve_session(context, "Vdd") would pass type checking, but at run time it translated "Vdd" to ["V", "d", "d"], which is surprising behavior.

The default timeout behavior was previously undocumented. Using a default value of None was ambiguous, because None could plausibly be interpreted as either -1 (infinite) or 0.0 (non-blocking).

What testing has been done?

Ran poetry run pytest -v.
Manually tested a modified version of nidmm_measurement.
Ran poetry run sphinx-build _docs_source docs -b html -W --keep-going and looked at the output.

@github-actions
Copy link

github-actions bot commented Sep 20, 2023

Test Results

     12 files  ±    0       12 suites  ±0   2m 21s ⏱️ +19s
   215 tests +  17     186 ✔️ +  17    29 💤 ±0  0 ±0 
2 568 runs  +204  2 220 ✔️ +204  348 💤 ±0  0 ±0 

Results for commit d5b70fa. ± Comparison against base commit ceda75a.

This pull request removes 3 and adds 20 tests. Note that renamed tests count towards both.
tests.unit.test_session_management ‑ test___reserve_all_registered_sessions___sends_request
tests.unit.test_session_management ‑ test___reserve_session___sends_request
tests.unit.test_session_management ‑ test___reserve_sessions___sends_request
tests.unit.test_session_management ‑ test___all_optional_args___reserve_all_registered_sessions___sends_request_with_args
tests.unit.test_session_management ‑ test___all_optional_args___reserve_session___sends_request_with_args
tests.unit.test_session_management ‑ test___all_optional_args___reserve_sessions___sends_request_with_args
tests.unit.test_session_management ‑ test___explicit_none___reserve_all_registered_sessions___sends_request_with_defaults
tests.unit.test_session_management ‑ test___explicit_none___reserve_session___sends_request_with_defaults
tests.unit.test_session_management ‑ test___explicit_none___reserve_sessions___sends_request_with_defaults
tests.unit.test_session_management ‑ test___infinite_timeout___reserve_all_registered_sessions___sends_request_with_infinite_timeout[-1.0]
tests.unit.test_session_management ‑ test___infinite_timeout___reserve_all_registered_sessions___sends_request_with_infinite_timeout[-1]
tests.unit.test_session_management ‑ test___infinite_timeout___reserve_session___sends_request_with_infinite_timeout[-1.0]
tests.unit.test_session_management ‑ test___infinite_timeout___reserve_session___sends_request_with_infinite_timeout[-1]
…

♻️ This comment has been updated with latest results.

@bkeryan bkeryan requested a review from dixonjoel September 21, 2023 14:17
@bkeryan bkeryan merged commit aa17b56 into main Sep 21, 2023
17 checks passed
@bkeryan bkeryan deleted the users/bkeryan/reserve-single-pin branch September 21, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants