Simplify CeleryKubernetesExecutor tests #13307
Conversation
boring-cyborg
bot
added
the
area:Scheduler
dstandish
force-pushed
the
simplify-CKE-tests
branch
from
2869603
to
3e1de26
Compare
|
Hi @dstandish mind checking the failures in K8S tests (https://github.com/apache/airflow/runs/1607640815) before we can further review? It should be related to the change introduced in #13289, and @potiuk has fixed it in #13316 . So simply rebasing to the latest master should help. |
Sure. Will be a bit slow checking it if you don't mind ;-) But should be getting back to you within today. |
dstandish
force-pushed
the
simplify-CKE-tests
branch
from
3e1de26
to
e214df3
Compare
|
thanks @XD-DENG, done |
|
there are also 3 related PRs... related because they all came up as i was checking out CeleryKubernetesExecutor |
| ignore_ti_state = False | ||
| pool = None | ||
| cfg_path = None | ||
| assert cke.has_task(None) == cke_has |
There was a problem hiding this comment.
This is different from the original assertion (assert cke.has_task(ti)). May you clarify a bit ?
| simple_task_instance.queue = 'non-kubernetes-queue' | ||
| @parameterized.expand( | ||
| [ | ||
| ('other-queue',), |
There was a problem hiding this comment.
nit: let's use the original queue name "'non-kubernetes-queue"? to be consistent with other related test cases, also being more explicit about what's tested here.
There was a problem hiding this comment.
how about 'any-queue' or 'any-other-queue' -- because the behavior were testing is the difference in handling between the kubernetes queue (there is only one) and any queue besides the kubernetes queue
or 'arbitrarily-named-queue' or 'could-be-anything':)
my only hesitation with 'non-kubernetes-queue' is that there's some possibility it could be interpreted to mean that there is a particular designated queue, namely 'non-kubernetes-queue' which routes to celery, and otherwise it goes to kubernetes queue
my making the naming more generic, or more obviously arbitrary, i think it adds to readability.
let me know what you think. happy to defer to your judgment on this.
There was a problem hiding this comment.
i went with 'any-other-queue'
|
|
||
| @parameterized.expand( | ||
| [ | ||
| ('non-kubernetes-queue',), |
There was a problem hiding this comment.
@dstandish I agree with your proposal. Then I assume this line should be updated as well?
There was a problem hiding this comment.
yup. didn't see it. done.
There was a problem hiding this comment.
Cool.
I was about to request for a rebase to the latest master, but I see you have already done that 👍
Overall in good shape now. It's a great PR which does make the tests much cleaner. I will do a final check and get back to you tomorrow. Cheers.
dstandish
force-pushed
the
simplify-CKE-tests
branch
from
9ea57b4
to
85bfc50
Compare
|
Apparently the CI failure is due to un-related reason. Will merge this first. |
XD-DENG
added
the
type:misc/internal
* Simplify CeleryKubernetesExecutor tests Co-authored-by: Daniel Standish <[email protected]> (cherry picked from commit 10be375)
Existing tests for CeleryKubernetesExecutor had some code duplication that made it somewhat more difficult to understand what was being tested.
Here I use parameterization to reduce code, simplify, and make intention more clear.
In the tests test_has_tasks and test_adopt_tasks I take the opportunity to add a little more coverage.