-
-
Notifications
You must be signed in to change notification settings - Fork 47
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 API tests for all supported environments #18
Add API tests for all supported environments #18
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.
Overall the tests look good, I just have a couple of minor questions
print() | ||
if terminated: | ||
break | ||
finally: |
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 not catch the exception and tell the user?
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.
Any exception would halt the program (and the stack trace would be printed to stderr). The try-finally block is there to ensure that the environment is closed.
from gymnasium import spaces | ||
from gymnasium.utils.env_checker import check_env | ||
from gymnasium.wrappers.flatten_observation import FlattenObservation | ||
|
||
import miniwob # noqa: F401 | ||
from miniwob.tests.utils import ALL_TESTABLE_MINIWOB_ENVS | ||
|
||
|
||
class TestGymAPI: |
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 looks completely fine, Im just more use to functional style testing rather than class based testing.
Is @pytest.fixture(params=...)
equal to @pytest.mark.parameterize("env_id", ALL_...)
?
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.
Yes, it should do the same thing.
miniwob/tests/utils.py
Outdated
@@ -0,0 +1,111 @@ | |||
"""Test utilities.""" | |||
|
|||
ALL_TESTABLE_MINIWOB_ENVS = [ |
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.
Damn that is alot of environments, is there anyway to make this programmatic? i.e., loop over the environment registry (gymnasium.registry
) for all env id with miniwob
in the env_id (more proper would be to check namespace
)
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 changed to reading from the registry.
|
||
# Throw errors on warnings, except for some specific warnings. | ||
filterwarnings = [ | ||
"error", |
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.
Why are we filtering errors?
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.
The "error" in filterwarnings
means to turn warnings into errors (source). I have edited the comment to clarify this.
Sorry for not responding earlier. I have addressed the comments. |
Thanks for the PR, looks good |
Description
For all registered environments (excluding FlightWoB), added tests for the following:
gymnasium.utils.env_checker.check_env
, which checks the observation space and seed determinism (exceptclick-pie
, where the UI position can jitter by a few pixels).Also added a utility for inspecting the observation space. Tasks were modified to pass the tests.
Addressed part of issue #15.
Type of change
Please delete options that are not relevant.
Checklist:
pre-commit
checks withpre-commit run --all-files
(seeCONTRIBUTING.md
instructions to set it up)