Fix flaky test_recover_from_resource_too_old exception #28475
Conversation
boring-cyborg
bot
added
the
area:Scheduler
potiuk
force-pushed
the
fix-failing-resource-version-test
branch
from
9c7e6fa
to
1d80b80
Compare
potiuk
changed the title
potiuk
changed the title
After apache#28047 the test_recover_from_resource_too_old started to fail in a flaky way. Turned out that - depend on some other test run the Singleton ResourceVersion could containt not one but two namespaces (including default namespace). Also while fixing the tests it's been noticed that the test missed an assert - it did not assert that the Exception was in fact thrown, so the test could have succeeded even if the exception was not really thrown (there was assert in "except" clause but if the exception was not thrown, it would not have been called at all).
potiuk
force-pushed
the
fix-failing-resource-version-test
branch
from
1d80b80
to
a72c542
Compare
| @@ -1249,12 +1249,13 @@ def effect(): | |||
| try: | |||
| # self.watcher._run() is mocked and return "500" as last resource_version | |||
| self.watcher.run() | |||
| assert False, "Should have raised Exception" | |||
There was a problem hiding this comment.
The change in line 1258 is all good to me.
But is this change at line 1252 necessary? I don't fully get it yet . The watcher is already mocked and will fail
There was a problem hiding this comment.
This is just consistency change. The author actually expected the sentinel exception (and it's the one that is raised form the watcher to break the loop) but the path when (potentially) no exception is thrown would not run this assert:
except Exception as e:
assert e.args == ("sentinel",)
Sure, we know what happens inside the run method and by inspecting it, we know it is not going to happen most likely - because exception is the only way to get out of the loop. But ... this might change in the future.
Not likely and stupid example but if someonedoes fast "return 0" in the watcher.run() - the test would have succeeded as well (no exception and resource = 0, assert e.args == ("sentinel", ) would have not been called).
Adding the assert simply makes absolutely sure that the exception was thrown (because otherwise assert would fail). That's the usual pattern :)
After #28047 the test_recover_from_resource_too_old started to fail in a flaky way. Turned out that - depend on some other test run the Singleton ResourceVersion could containt not one but two namespaces (including default namespace).
Also while fixing the tests it's been noticed that the test missed an assert - it did not assert that the Exception was in fact thrown, so the test could have succeeded even if the exception was not really thrown (there was assert in "except" clause but if the exception was not thrown, it would not have been called at all).
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.