Skip to content

Conversation

@jayaseelan-james
Copy link
Contributor

What does this Pull Request accomplish?

Includes a new nidcpower_measurement.py test service module and adds acceptance and integration tests for nidcpower driver specific session management APIs.

Why should this Pull Request be merged?

To add nidcpower driver-specific tests to the repository.

What testing has been done?

Ran mypy and pytest.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 30, 2023

Test Results

       30 files  ±  0         30 suites  ±0   41m 22s ⏱️ + 5m 34s
     491 tests +  7       491 ✔️ +  7         0 💤 ±0  0 ±0 
12 510 runs  +70  11 450 ✔️ +70  1 060 💤 ±0  0 ±0 

Results for commit 0fd1300. ± Comparison against base commit 71ffb3d.

This pull request removes 9 and adds 16 tests. Note that renamed tests count towards both.
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.acceptance.test_nidcpower_measurement ‑ test___multiple_sessions___measure___creates_multiple_sessions
tests.acceptance.test_nidcpower_measurement ‑ test___single_session___measure___creates_single_session
tests.acceptance.test_nidcpower_measurement ‑ test___single_session___measure___returns_measured_values
tests.integration.session_management.test_nidcpower_reservation ‑ test___multiple_sessions_reserved___initialize_nidcpower_sessions___creates_multiple_sessions
tests.integration.session_management.test_nidcpower_reservation ‑ test___session_created___get_nidcpower_connection___returns_connection
tests.integration.session_management.test_nidcpower_reservation ‑ test___sessions_created___get_nidcpower_connections___returns_connections
tests.integration.session_management.test_nidcpower_reservation ‑ test___single_session_reserved___initialize_nidcpower_session___creates_single_session
tests.integration.session_management.test_reservation ‑ test___sessions_reserved___get_connections___connections_returned
tests.integration.session_management.test_reservation ‑ test___sessions_reserved___get_connections_by_instrument_type___connections_returned
tests.integration.session_management.test_reservation ‑ test___sessions_reserved___get_connections_by_pin___connections_returned
…

♻️ This comment has been updated with latest results.

@dixonjoel
Copy link
Collaborator

dixonjoel commented Oct 30, 2023

This doesn't cover all the cases mentioned in AB#2535722. Are you planning to add the rest of the tests? With the RFC, what questions are you hoping to get feedback on?

It also looks like the system tests are failing because there's no DCPower1 on the test runner. Will we need to add this DC instrument to the test runner's setup? Or maybe we can use a .env file to set up simulation for this test?

@dixonjoel
Copy link
Collaborator

If you can get the system tests passing and cover more of the desired tests, this approach looks ok to me. So I'm approving for now. Reset me if you're ready to remove the RFC tag.

Copy link
Collaborator

@dixonjoel dixonjoel left a comment

Choose a reason for hiding this comment

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

Approved for RFC

@jayaseelan-james
Copy link
Contributor Author

jayaseelan-james commented Oct 31, 2023

This doesn't cover all the cases mentioned in AB#2535722. Are you planning to add the rest of the tests?

I was working to come up with the initial setup for the integration and acceptance tests. That was the reason I had the [Internal] and [RFC] tags to get inputs from the Soliton team but since the pipeline failed, I didn't remove the [Internal] tag yesterday.

With the RFC, what questions are you hoping to get feedback on?

I added it mainly to confirm the testing approach for the nidcpower session APIs, so that I can proceed with the tests for other drivers.

It also looks like the system tests are failing because there's no DCPower1 on the test runner. Will we need to add this DC instrument to the test runner's setup? Or maybe we can use a .env file to set up simulation for this test?

Yes, we're working on updating the pipeline to have the .env file in place before running acceptance tests.

I'm removing the RFC tag as all the required tests are added now but will add a Review tag until the pipeline gets fixed. Hope that's fine. Apologies for any confusion caused.

@dixonjoel, I'm resetting your vote here as the pipeline update is taken up as a separate PR.

@jayaseelan-james jayaseelan-james changed the title [Internal] [RFC] tests: Add integration and acceptance for NI-DCPower driver specific session management APIs [Review] tests: Add integration and acceptance tests for NI-DCPower driver specific session management APIs Oct 31, 2023
Copy link
Collaborator

@bkeryan bkeryan left a comment

Choose a reason for hiding this comment

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

This PR:

  • package ni.measurementlink.measurement.tests.nidcpower_measurement
  • tests/assets/stubs/nidcpower_measurement/types.proto
  • Run generate_grpc_stubs.py, commit nidcpower_measurement changes, revert the rest.
  • Update imports to match.

Follow-up PRs:

  • Move existing test proto files and regenerate in new location.
  • Regenerate all existing .pyi files.
  • Update check_nims.yml to check proto codegen.

@jayaseelan-james jayaseelan-james changed the title [Review] tests: Add integration and acceptance tests for NI-DCPower driver specific session management APIs tests: Add integration and acceptance tests for NI-DCPower driver specific session management APIs Nov 3, 2023
@jayaseelan-james jayaseelan-james merged commit 01dff89 into main Nov 3, 2023
@jayaseelan-james jayaseelan-james deleted the users/jay/tests-for-nidcpower branch November 3, 2023 15:35
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.

5 participants