-
Notifications
You must be signed in to change notification settings - Fork 17
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
add first pytest unit tests #100
Conversation
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 think the current version is good to merge but added some cmments regarding making the path more configurable and adding cache for the conda env to make the workflow faster.
from pathlib import Path | ||
|
||
|
||
_TEMPLATE_PATH = Path(__file__).parents[3] / "template" |
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.
For these path, should we consider using some more configurable formats?
Something like:
from pathlib import Path
from pyrcareworld.envs.base_env import RCareWorld
class DressingEnv(RCareWorld):
"""Dressing environment for the PhyRC competition."""
def __init__(self, template_path=None, executable_path=None, *args, **kwargs):
# Allow paths to be passed as arguments or use defaults
template_path = Path(template_path or Path(__file__).parents[3] / "template")
executable_path = Path(executable_path or template_path / "Dressing" / "DressingPlayer.x86_64")
super().__init__(executable_file=str(executable_path), *args, **kwargs)
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.
Thanks for the feedback! I made executable_path
configurable, but I figure that we probably won't need to configure template_path
separately from that.
.github/workflows/ci.yml
Outdated
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.
Should we cache the conda env?
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.
This is a good idea, but I couldn't figure out exactly the right way to do this after looking at it for a bit, so I'll save this for a future PR!
also make bathing and dressing environments easier to create