Refactor CompositeSpec tests #230
Conversation
facebook-github-bot
added
the
CLA Signed
|
Thanks for this! Let's have a look at the failed tests. |
|
Yes, looking at this right now. Unfortunately, I wasn't able to make suggested pre-commit linter work, but I ran some linters separately, and that should fix most issues. |
vmoens
changed the title
Fix mock envs dtype casting
Device check for bounded space
test/test_tensor_spec.py
Outdated
| def test_setitem_matches_device(self, is_complete, device, dtype): | ||
| ts = self._composite_spec(is_complete, device, dtype) | ||
|
|
||
| bad_value, good_value = Mock(), Mock() |
There was a problem hiding this comment.
Great idea of using Mock though I see two caveats:
At one point we might check that specs are or type TensorSpec, does mock works in that case? (Ie does isinstance(mock, type) always true for all types?)
Second we rely on pytest for our tests and I'm not sure if we want to mix unittest and pytest.
There was a problem hiding this comment.
Good points, I'll change this.
test/test_tensor_spec.py
Outdated
| ts = self._composite_spec(is_complete, device, dtype) | ||
| assert "obs" in ts.keys() | ||
| assert "act" in ts.keys() | ||
| ts.del_("obs") |
There was a problem hiding this comment.
Can we try with
del ts["obs"]
Too?
There was a problem hiding this comment.
Sure! Though I'm curious why we need del_ then, can we simply rename it to __delitem__?
There was a problem hiding this comment.
sure! If tests pass i'm ok with it :p
#220