Skip to content

Comments

1163 support signal[t] | none syntax in devicefiller#1165

Merged
coretl merged 6 commits intomainfrom
1163-support-signalt-none-syntax-in-devicefiller
Dec 18, 2025
Merged

1163 support signal[t] | none syntax in devicefiller#1165
coretl merged 6 commits intomainfrom
1163-support-signalt-none-syntax-in-devicefiller

Conversation

@LuisFSegalla
Copy link
Contributor

Addresses #1163

@LuisFSegalla LuisFSegalla requested a review from coretl December 17, 2025 09:56
@LuisFSegalla LuisFSegalla linked an issue Dec 17, 2025 that may be closed by this pull request
async def connect_mock(self, device: Device, mock: LazyMock):
self.filler.create_device_vector_entries_to_mock(self.mock_device_vector_len)
# Set the name of the device to name all children
self.filler.check_filled("")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed? All optional devices should have been created in mock mode so there should be no need to call this function...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this so that when trying to connect check_filled would be called testing the added code to create the disconnected attr. Otherwise no other part of the mock code calls for filler.check_filled().

Copy link
Collaborator

@coretl coretl Dec 17, 2025

Choose a reason for hiding this comment

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

Ok, please remove this line and add a test that instantiates DeviceFiller manually on a Device with both mandatory and optional Signals, then calls check_filled to check its failure and success paths.

await device.connect(mock=True)

# After connecting if the signal is not present it's set to None
assert device.signal_rw is None
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would expect the signal to exist here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that was my mistake, I'll fix it.

@coretl
Copy link
Collaborator

coretl commented Dec 17, 2025

@claude suggest code for the test mentioned in #1165 (comment)

@claude
Copy link

claude bot commented Dec 17, 2025

Claude finished @coretl's task in 2m 40s —— View job


Test implementation completed

Tasks:

  • Read the modified files to understand the implementation
  • Read existing tests to understand test patterns
  • Remove line at _pvi_connector.py:117 as requested
  • Create new test that instantiates DeviceFiller manually with mandatory and optional Signals
  • Test both success and failure paths of check_filled
  • Commit and push changes

Changes Made

I've implemented the test as requested in the review comment:

1. Removed unnecessary line from _pvi_connector.py:117

  • Removed the self.filler.check_filled("") call from connect_mock method

2. Added comprehensive test in test_device.py

  • Created test_device_filler_check_filled_with_optional_signals()
  • Manually instantiates DeviceFiller with a test Device containing:
    • mandatory_signal: SignalRW[int] - a required signal
    • optional_signal: SignalRW[int] | None - an optional signal
  • Tests failure path: check_filled raises RuntimeError when mandatory signal is unfilled
  • Tests success path: After filling mandatory signal, check_filled succeeds and sets optional signal to None

3. Updated comment in test_pvi.py

  • Clarified that the test checks optional signal behavior

The test verifies that DeviceFiller correctly:

  • Identifies signals with T | None annotations as optional
  • Raises errors when mandatory signals are unfilled
  • Sets optional signals to None when they remain unfilled
  • Allows successful connection when all mandatory signals are filled

--- | Branch

- Remove unnecessary check_filled call from PviDeviceConnector.connect_mock
- Add comprehensive test for DeviceFiller.check_filled testing both success
  and failure paths with mandatory and optional signals
- Update test comment to clarify optional signal behavior

Co-authored-by: Tom C (DLS) <coretl@users.noreply.github.com>
@LuisFSegalla LuisFSegalla changed the title 1163 support signalt none syntax in devicefiller 1163 support signal[t] | none syntax in devicefiller Dec 18, 2025
* Updated pvi test to show that signal before and after connect still
exists
	* Updated device test so that it would check for optional signal before
and after filling mandatory signals.

if unfilled_mandatory:
raise RuntimeError(
f"{self._device.name}: cannot provision {unfilled} from {source}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
f"{self._device.name}: cannot provision {unfilled} from {source}"
f"{self._device.name}: cannot provision {unfilled_mandatory} from {source}"

@coretl coretl marked this pull request as ready for review December 18, 2025 13:35
@coretl coretl merged commit dcb671b into main Dec 18, 2025
56 of 60 checks passed
@coretl coretl deleted the 1163-support-signalt-none-syntax-in-devicefiller branch December 18, 2025 13: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.

Support Signal[T] | None syntax in DeviceFiller

2 participants