Skip to content
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

World knowledge tests #126

Merged
merged 14 commits into from
Jul 30, 2023
Merged

World knowledge tests #126

merged 14 commits into from
Jul 30, 2023

Conversation

henrygerardmoore
Copy link
Collaborator

Added world knowledge tests. Not entirely sure if this is a good format or if we want more or different kinds of tests, feel free to ask for any of that! This is my first time making python tests so it will be a learning experience

Resolves #99

Copy link
Owner

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

I have some high-level suggestions, in addition to / to summarize the inline comments:

  1. I just realized that apply_resolution_strategy doesn't actually use the world argument? We can remove it.
  2. Add docstrings to each test function explaining what is being tested
  3. Check out my comment on using pytest.raises -- will save a lot of lines of code
  4. Instead of using Pose.from_list() just use the regular constructor with named arguments, e.g. Pose(x=1.0, y=2.0, yaw=np.pi/2)

pyrobosim/pyrobosim/utils/knowledge.py Outdated Show resolved Hide resolved
test/utils/test_knowledge_utils.py Outdated Show resolved Hide resolved
test/utils/test_knowledge_utils.py Outdated Show resolved Hide resolved
test/utils/test_knowledge_utils.py Show resolved Hide resolved
test/utils/test_knowledge_utils.py Outdated Show resolved Hide resolved
test/utils/test_knowledge_utils.py Show resolved Hide resolved
test/utils/test_knowledge_utils.py Outdated Show resolved Hide resolved
test/utils/test_knowledge_utils.py Outdated Show resolved Hide resolved
test/utils/test_knowledge_utils.py Outdated Show resolved Hide resolved
test/utils/test_knowledge_utils.py Outdated Show resolved Hide resolved
@henrygerardmoore
Copy link
Collaborator Author

@sea-bass made all the changes except the pytest-assume one, which I commented on above. I also broke up another of the larger test into its conceptual parts, hopefully now it's easier to parse what's going on in the tests.

Copy link
Owner

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Looks awesome -- thanks @henrygerardmoore!

Comment on lines +133 to +137
assert (
entity.category == "banana"
and entity.parent.category == "table"
and entity.parent.parent.parent.name == "kitchen"
)
Copy link
Owner

Choose a reason for hiding this comment

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

I just tried locally and it worked great for me...

You can either just do pip install pytest-assume in your existing environment, or if you add it to the requirements file you'll need to rerun the entire setup/create_python_env.bash.

... but like, this doesn't matter for this PR -- it was just an out there suggestion. I'll let you follow up in another if you care!

@sea-bass sea-bass merged commit 532208c into sea-bass:main Jul 30, 2023
5 checks passed
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 unit tests for world knowledge utilities
2 participants