add CI test for auto plugin #5504
Conversation
ci: no-compile
| def test_invalid_period_type(self): | ||
| """Test invalid input types.""" | ||
| with self.assertRaises(typeguard.TypeCheckError): | ||
| Auto(period="invalid") |
There was a problem hiding this comment.
What's the difference between this line and line 57+58?
And why are both tests needed?
There was a problem hiding this comment.
The key difference is when and how the invalid value is introduced.
test_invalid_period_type is done During def init. Auto is decorated with @typeguard.typechecked, typeguard automatically checks the type of auto period. so the error happens during object creation.
test_check_invalid_period is During check(). first we create a valid object: Auto(period=10). then, we manually break it by assigning an invalid value to auto.period.. Typeguard does not automatically recheck assignments. auto.check(), then raises a TypeError because period is no longer a TimeStepSpec.
chillenzer
added
component: tests
There was a problem hiding this comment.
Thanks for your effort. Unit testing really important to maintain a high quality code base, so this is a valuable attempt.
In the details, the PR attempts to hand responsibilities to Auto that it really shouldn't have. It should delegate everything related to TimeStepSpec to TimeStepSpec because, you know, that's what it is actually related to. As it afterwards doesn't have any code logic in it, I don't see what could possibly be tested about it.
However, the best implementation would be have Auto as a shallow convenience interface like this
class Auto(Png):
def __init__(self, period):
super().__init__(
period=period,
# put some default arguments here
)This would get rid of the special treatment of Auto, remove a lot of code in the process and clearly communicate to the user what Auto actually does.
| if isinstance(period, int): | ||
| if period < 0: | ||
| raise ValueError("period must be non-negative") | ||
| self.period = TimeStepSpec[::period]("steps") if period > 0 else TimeStepSpec([])("steps") | ||
| else: | ||
| self.period = period |
There was a problem hiding this comment.
This is not the auto plugin's responsitbility. If we want to have such a check, we should have it in TimeStepSpec. But keep in mind that this kind of constraint is supposed to be codified as part of a pydantic validation in the future.
| def test_negative_period(self): | ||
| """Test negative integer period raises error.""" | ||
| with self.assertRaisesRegex(ValueError, "period must be non-negative"): | ||
| Auto(period=-5) |
| def check(self, *args, **kwargs): | ||
| """Validate that period is a valid TimeStepSpec.""" | ||
| if not isinstance(self.period, TimeStepSpec): | ||
| raise TypeError("period must be a TimeStepSpec") |
There was a problem hiding this comment.
This duplicates the check in the __init__ method. I'd rather do something like
def __init__(self, ...):
# ...
self.check()| if not isinstance(period, (int, TimeStepSpec)): | ||
| raise TypeError("period must be an integer or TimeStepSpec") | ||
| if isinstance(period, int): | ||
| if period < 0: | ||
| raise ValueError("period must be non-negative") | ||
| self.period = TimeStepSpec[::period]("steps") if period > 0 else TimeStepSpec([])("steps") | ||
| else: | ||
| self.period = period |
There was a problem hiding this comment.
I'd rather hand anything given here over to TimeStepSpec and make that interpret the request. This ensures that there is a consistent conversion logic, error handling, etc. across all using classes of TimeStepSpec.
|
@mafshari64 What is the status of this PR? |
|
Currently, I am working on web design and hope to finish it in 1-2 weeks. Then fully focus on these PRs. |
|
Thanks for the update. Converting to draft for the moment. |
3 participants
ci: no-compile