-
Notifications
You must be signed in to change notification settings - Fork 34
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
Instantiate Mock members by function #523
Conversation
44b72ff
to
74aae96
Compare
Format files Make MockStep methods static
74aae96
to
2c1903e
Compare
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.
Well spotted that you found the cause of the issue. Some comments for naming:
return DriveToPose(pose=MockPose.default_pose) | ||
|
||
@staticmethod | ||
def take_image_in_coordinate_direction() -> TakeImage: |
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.
def take_image_in_coordinate_direction() -> TakeImage: | |
def get_dummy_take_image_step() -> TakeImage: |
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.
See below regarding naming. It is added as a separate issue.
@@ -5,7 +5,10 @@ | |||
|
|||
|
|||
class MockStep: |
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.
class MockStep: | |
class DummyStep: |
Also consider using the naming "Stub" or "Fake", both may be representative of what we try to accomplish here. Relevant article: https://jesusvalerareales.com/testing-with-test-doubles/
Also, does not need to be solved here, you can make an issue for it and suggest a renaming for all similar "Mocks" that should be called "Dummy", "Stub", or "Fake"
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.
Good idea, and great tip for a relevant resource. I will defer this to a separate issue, and have created one.
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.
Good work 👍 Wise to delegate comments to a separate issue
Fixes bug in state machine tests due to class instance member being mutated across different tests.
Resolves #516