potiuk commented on code in PR #66423:
URL: https://github.com/apache/airflow/pull/66423#discussion_r3194467285
##########
kubernetes-tests/tests/kubernetes_tests/test_kubernetes_pod_operator.py:
##########
@@ -218,7 +218,7 @@ def test_config_path_move(self, kubeconfig_path, tmp_path):
def test_working_pod(self):
k = KubernetesPodOperator(
namespace="default",
- image="ubuntu",
+ image="ubuntu:24.04",
Review Comment:
I think using global variables is overrated in the age of AI - especially in
tests - and especially that we have "prek" hook that will update them for us :).
In tests DAMP is often better than DRY - this has been widely recognized
fact even before AI agents - and there are even less arguments now to extract
constrants in tests, when your agent will automatically keep it consistent (And
in our case we even have determinitic hook that will do it for you).
This article has good explanation why:
https://enterprisecraftsmanship.com/posts/dry-damp-unit-tests/
My simple translation of DAMP vs. DRY is "You should not need to look
elsewhere to understand what the test does" - and this also means that tests
should not really use constants (unless this is some complex calculation) -
declared elsewhere especially if the constant names do not describe what it is.
In this case - if I apply that rule - I would have to declare that constant
as
```python
UBUNITU_24_04 = "ubuntu:24.04"
```
(and to be honest - that makes very little sense and no saving)
In order to know what I am looking at.
If I do that instead:
```python
IMAGE="ubuntu:24.04"
```
The test will loos some information - which image we run it on, and you have
to look elsewhere to find out - this is not good for human and also it's not
good for `AGENTS` - because agents cannot determine the whole context of what
the test does by just grepping the test and looking at it's definition - it
will also have to look at the constant definition - so basically it will have
to parse the whole test in order to find out what image we are running the test
with. Which is more tokens and time.
Performance-wise - all those strings are interned anyway (there is one
string in Python memory "ubuntu:24.04" that is reused because strings in Python
are immutable and interned by default. So we are not loosing performance/memory
here as well.
So ... I think this is is pretty good idea to leave the explicit image names
as strings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
