-
Couldn't load subscription status.
- Fork 20
Make sure base_value is always float
#874
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,12 +3,15 @@ | |
|
|
||
| """Tests for quantity types.""" | ||
|
|
||
| import inspect | ||
| from datetime import timedelta | ||
| from typing import Callable | ||
|
|
||
| import hypothesis | ||
| import pytest | ||
| from hypothesis import strategies as st | ||
|
|
||
| from frequenz.sdk.timeseries import _quantities | ||
| from frequenz.sdk.timeseries._quantities import ( | ||
| Current, | ||
| Energy, | ||
|
|
@@ -45,6 +48,48 @@ class Fz2( | |
| """Frequency quantity with broad exponent unit map.""" | ||
|
|
||
|
|
||
| _CtorType = Callable[[float], Quantity] | ||
|
|
||
| # This is the current number of subclasses. This probably will get outdated, but it will | ||
| # provide at least some safety against something going really wrong and end up testing | ||
| # an empty list. With this we should at least make sure we are not testing less classes | ||
| # than before. We don't get the actual number using len(_QUANTITY_SUBCLASSES) because it | ||
| # would defeat the purpose of the test. | ||
| _SANITFY_NUM_CLASSES = 7 | ||
|
|
||
| _QUANTITY_SUBCLASSES = [ | ||
| cls | ||
| for _, cls in inspect.getmembers( | ||
| _quantities, | ||
| lambda m: inspect.isclass(m) and issubclass(m, Quantity) and m is not Quantity, | ||
| ) | ||
| ] | ||
|
|
||
| # A very basic sanity check that are messing up the introspection | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Someone def. messed up this comment :) |
||
| assert len(_QUANTITY_SUBCLASSES) >= _SANITFY_NUM_CLASSES | ||
|
|
||
| _QUANTITY_BASE_UNIT_STRINGS = [ | ||
| cls._new(0).base_unit # pylint: disable=protected-access | ||
| for cls in _QUANTITY_SUBCLASSES | ||
| ] | ||
| for unit in _QUANTITY_BASE_UNIT_STRINGS: | ||
| assert unit is not None | ||
|
|
||
| _QUANTITY_CTORS = [ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So close, we could have called these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I made my first meme-PR! |
||
| method | ||
| for cls in _QUANTITY_SUBCLASSES | ||
| for _, method in inspect.getmembers( | ||
| cls, | ||
| lambda m: inspect.ismethod(m) | ||
| and m.__name__.startswith("from_") | ||
| and m.__name__ != ("from_string"), | ||
| ) | ||
| ] | ||
| # A very basic sanity check that are messing up the introspection. There are actually | ||
| # many more constructors than classes, but this still works as a very basic check. | ||
| assert len(_QUANTITY_CTORS) >= _SANITFY_NUM_CLASSES | ||
|
|
||
|
|
||
| def test_zero() -> None: | ||
| """Test the zero value for quantity.""" | ||
| assert Quantity(0.0) == Quantity.zero() | ||
|
|
@@ -101,6 +146,31 @@ def test_zero() -> None: | |
| assert Percentage.zero() is Percentage.zero() # It is a "singleton" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("quantity_ctor", _QUANTITY_CTORS) | ||
| def test_base_value_from_ctor_is_float(quantity_ctor: _CtorType) -> None: | ||
| """Test that the base value always is a float.""" | ||
| quantity = quantity_ctor(1) | ||
| assert isinstance(quantity.base_value, float) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("quantity_type", _QUANTITY_SUBCLASSES + [Quantity]) | ||
| def test_base_value_from_zero_is_float(quantity_type: type[Quantity]) -> None: | ||
| """Test that the base value always is a float.""" | ||
| quantity = quantity_type.zero() | ||
| assert isinstance(quantity.base_value, float) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "quantity_type, unit", zip(_QUANTITY_SUBCLASSES, _QUANTITY_BASE_UNIT_STRINGS) | ||
| ) | ||
| def test_base_value_from_string_is_float( | ||
| quantity_type: type[Quantity], unit: str | ||
| ) -> None: | ||
| """Test that the base value always is a float.""" | ||
| quantity = quantity_type.from_string(f"1 {unit}") | ||
| assert isinstance(quantity.base_value, float) | ||
|
|
||
|
|
||
| def test_string_representation() -> None: | ||
| """Test the string representation of the quantities.""" | ||
| assert str(Quantity(1.024445, exponent=0)) == "1.024" | ||
|
|
@@ -539,6 +609,9 @@ def test_invalid_multiplications() -> None: | |
| quantity *= 200.0 # type: ignore | ||
|
|
||
|
|
||
| # We can't use _QUANTITY_TYPES here, because it will break the tests, as hypothesis | ||
| # will generate more values, some of which are unsupported by the quantities. See the | ||
| # test comment for more details. | ||
| @pytest.mark.parametrize("quantity_type", [Power, Voltage, Current, Energy, Frequency]) | ||
| @pytest.mark.parametrize("exponent", [0, 3, 6, 9]) | ||
| @hypothesis.settings( | ||
|
|
@@ -563,8 +636,14 @@ def test_to_and_from_string( | |
| generation and regenerate it with the more appropriate unit and precision. | ||
| """ | ||
| quantity = quantity_type.__new__(quantity_type) | ||
| # pylint: disable=protected-access | ||
| quantity._base_value = value * 10**exponent | ||
| quantity._base_value = value * 10**exponent # pylint: disable=protected-access | ||
| # The above should be replaced with: | ||
| # quantity = quantity_type._new( # pylint: disable=protected-access | ||
| # value, exponent=exponent | ||
| # ) | ||
| # But we can't do that now, because, you guessed it, it will also break the tests | ||
| # (_new() will use 10.0**exponent instead of 10**exponent, which seems to have some | ||
| # effect on the tests. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By "on the tests", I guess you mean just this test, because hypothesis also generates some int inputs when we give it a float range? This function seems to just compare the value from And then they can be compared with |
||
| quantity_str = f"{quantity:.{exponent}}" | ||
| from_string = quantity_type.from_string(quantity_str) | ||
| try: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit late to the review, but it might be better to call this
_SANITIFY_NUM_CLASSES.. I just don't see a reason to skip this extra I...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just try to pronounce it with and without :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree completely. When making typos, please make sure you don't skimp on the vowels. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea how I ended up typing this. But luckily we have smart completion, so typos are properly propagated and the code never fails!