Allow deprecation for freezegun #3950
Conversation
balopat
requested review from
cduck,
vtomole,
wcourtney and
a team
as code owners
| try: | ||
| return func(*args, **kwargs) | ||
| finally: | ||
| del os.environ[ALLOW_DEPRECATION_IN_TEST] |
There was a problem hiding this comment.
If someone runs check/pytest with ALLOW_DEPRECATION_IN_TEST enabled in their environment, wouldn't this disable it for all subsequent tests? I think we need to capture its value prior to this test and reinstate the same value afterwards.
There was a problem hiding this comment.
It would, but it is not meant to be enabled for all tests, across the board, intentionally.
We have the cirq.testing.assert_deprecated context manager that allows for executing deprecated code. I do not want to make it easy to escape from running deprecated code in Cirq itself.
There was a problem hiding this comment.
It would, but it is not meant to be enabled for all tests, across the board, intentionally.
This won't stop devs from using it this way anyways 😛 . I found it useful to enable ALLOW_DEPRECATION_IN_TEST for the proto-serialization changes to separate deprecation failures from behavioral failures; I suspect other devs will use it this way as well.
Even if we don't want to treat this as a valid use case, I think it's worthwhile to guard against "spooky errors at a distance" like this. Unit tests should never fail due to the order in which they are run.
There was a problem hiding this comment.
This won't stop devs from using it this way anyways 😛 .
Those cheeky developers.
I found it useful to enable
ALLOW_DEPRECATION_IN_TESTfor the proto-serialization changes to separate deprecation failures from behavioral failures; I suspect other devs will use it this way as well.
Fair - being able to tackle piecemeal what is yelling at you sounds like a good feature.
There was a problem hiding this comment.
Unit tests should never fail due to the order in which they are run.
That is a good point too. I'll revert to the previous state of it.
balopat
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
Allows usage of deprecated functionality for test functions leveraging freezegun.
During cirq.google migration this came up - as we don't have yet any deprecated attributes it didn't bite us.
Note: all other alternatives are significantly more complex, for example