Skip to content

Add tests for derived signal #850

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

Merged
merged 20 commits into from
Jun 11, 2025
Merged

Add tests for derived signal #850

merged 20 commits into from
Jun 11, 2025

Conversation

Villtord
Copy link
Contributor

@Villtord Villtord commented Apr 4, 2025

Added tests to improve coverage for _derived_signal_backend and _derived_signal.
It looks like I shouldn't have done a rebase locally as now there are files from main branch which I didn't change but they have been merged already, sorry about it.

eir17846 and others added 7 commits March 31, 2025 16:16
#846)

* Add test that checks to make sure put completion works for PVA signals

* Use pytest.approx for timing

* Adjust tolerance

* Even tighter tolerance

* Parametrize put completion test by protocol, make put completion wait 0.5 seconds instead of 5
@Villtord
Copy link
Contributor Author

@coretl I think i got rid of all private imports, however I still have to get to certain methods addressing _methods in tests, I am not sure how to avoid it tbh.

@Villtord Villtord self-assigned this Apr 30, 2025
Comment on lines 142 to 152
def derived_signal_backend() -> SignalBackend[SignalDatatype]:
signal_r = soft_signal_rw(int, initial_value=4)

def _get(ts: int) -> float:
return ts

async def _put(value: float) -> None:
pass

derived = derived_signal_rw(_get, _put, ts=signal_r)
return derived._get_cache().backend
Copy link
Collaborator

Choose a reason for hiding this comment

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

Continuing with the theme of testing the public interface, can we make this return the whole signal instead of the backend?
...

Comment on lines 155 to 159
async def test_derived_signal_backend_connect_pass(
derived_signal_backend: SignalBackend,
):
result = await derived_signal_backend.connect(0.1)
assert result 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.

Then this would be

Suggested change
async def test_derived_signal_backend_connect_pass(
derived_signal_backend: SignalBackend,
):
result = await derived_signal_backend.connect(0.1)
assert result is None
async def test_derived_signal_backend_connect_pass(
derived_signal: SignalRW,
):
result = await derived_signal.connect()
assert result is None

Comment on lines 169 to 173
async def test_derived_signal_backend_put_wait_fails(
derived_signal_backend: SignalBackend,
) -> None:
with pytest.raises(RuntimeError):
await derived_signal_backend.put(value=None, wait=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

And this would be

Suggested change
async def test_derived_signal_backend_put_wait_fails(
derived_signal_backend: SignalBackend,
) -> None:
with pytest.raises(RuntimeError):
await derived_signal_backend.put(value=None, wait=False)
async def test_derived_signal_backend_put_wait_fails(
derived_signal: SignalRW,
) -> None:
with pytest.raises(RuntimeError):
await derived_signal.set(value=None, wait=False)

Comment on lines 162 to 166
def test_derived_signal_backend_set_value(
derived_signal_backend: SignalBackend,
) -> None:
with pytest.raises(RuntimeError):
derived_signal_backend.set_value(1.0) # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit trickier to test, but still possible with the public interface

Suggested change
def test_derived_signal_backend_set_value(
derived_signal_backend: SignalBackend,
) -> None:
with pytest.raises(RuntimeError):
derived_signal_backend.set_value(1.0) # type: ignore
def test_derived_signal_backend_set_value(
derived_signal: SignalRW,
) -> None:
await derived_signal.connect(mock=True)
with pytest.raises(RuntimeError):
set_mock_value(derived_signal, 1.0)

ValueError,
match=re.escape("Must define a set_derived method to support derived"),
):
factory._make_signal(signal_cls=SignalRW, datatype=Table, name="")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Public interface is

Suggested change
factory._make_signal(signal_cls=SignalRW, datatype=Table, name="")
factory.derived_signal_rw(datatype=Table, name="")

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think all of these are possible without using patch or MagicMock or referring to private members of the variable, but let me know if any of them are difficult and I'll have a think how...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have to say am really struggling trying to find the way around using public interface without mock for e.g. ?

async def test_set_derived_not_initialized(derived_signal_backend: SignalBackend):
    with patch.object(
        derived_signal_backend.transformer,  # type: ignore
        "_set_derived",
        None,
    ):
        with pytest.raises(RuntimeError):
            await derived_signal_backend.put("name", True)

any ideas on that one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I don't think you can test that one with the public interface. Best I can do is:

async def test_set_derived_not_initialized():
    def _get(ts: int) -> float:
        return ts

    sig = derived_signal_r(_get, ts=soft_signal_rw(int, initial_value=4))
    with pytest.raises(
        RuntimeError,
        match="Cannot put as no set_derived method given",
    ):
        await sig._connector.backend.put(1.0, True)

If you've tried using the public interface and it's not possible then I'm happy with patches or private member variables

Copy link
Collaborator

@coretl coretl left a comment

Choose a reason for hiding this comment

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

One comment to remove then good to merge, thanks!

return filter_by_type(self._raw_devices | self._transform_devices, Subscribable)
return validate_by_type(
self._raw_devices | self._transform_devices, Subscribable
) # noqa: E501
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
) # noqa: E501
)

@Villtord Villtord linked an issue Jun 11, 2025 that may be closed by this pull request
@coretl coretl merged commit 4acb229 into main Jun 11, 2025
27 checks passed
@coretl coretl deleted the Add-tests-for-DerivedSignal branch June 11, 2025 16:01
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.

Add tests for "unhappy path" of DerivedSignal
3 participants