Migrating helm chart tests to helm_tests to prevent test DB init #32394

amoghrajesh · 2023-07-06T07:59:25Z

Seems that pytest will walk up looking for other conftest.py files beyond the tests/charts repo due to which the helm chart tests require DB init to be done which should not be the case. The tests inside tests finds the nearest _clear_db fixture and continues to use it. We should not use that conftest fixture for helm chart tests.
So likely then #28631 had side-effect of trying to clear db where it was not initialized

This PR moves the helm charts tests outside tests/charts to test-charts. Tested using the steps:

Created a new env
Ran the command
pip install -e ."[devel]" --constraint https://raw.githubusercontent.com/apache/airflow/constraints-main/constraints-source-providers-3.8.txt
Ran the tests in test-charts and we are good 👍! Did not have to init any DB 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.rst or {issue_number}.significant.rst, in newsfragments.

potiuk

Loooks good - pending static checks fixes. Thanks for taking care of it :). Good idea with adding docstrings.

potiuk

Ah. Forgot it. Few more things are needed:

.dockerignore -> it should be added to not be excluded when Docker image is built (see !tests, !kubernetes_tests). Probably also adding !docker_tests is a good idea. We do not run docker tests in breeze usually, but having them in CI image would be nice.
you also need to add it to VOLUMES_FOR_SELECTED_MOUNTS in docker_command_utils.py in breeze - this way the folder will be also mounted when you run breeze command from local sources and you will be able to run the tests with latest sources when you are inside of breeze. Once you add it there, pre-commit will automatically update it in other places.
probably helm_tests is a better name after looking theere - it will be following the convention we already have there (docker_tests, kubernetes_tests)

amoghrajesh · 2023-07-06T09:26:11Z

All these points are valid. Making the changes to accommodate them

potiuk · 2023-07-06T09:58:38Z

All these points are valid. Making the changes to accommodate them

still some "test-charts" in there.

amoghrajesh · 2023-07-06T10:06:51Z

Woops. Looks like the IDE didnt catch it all. Made them manually

potiuk · 2023-07-06T11:11:38Z

Looks good - only static checks need to be fixed

amoghrajesh · 2023-07-06T11:22:32Z

Not sure what the failure is. doesn't look related to the changes in this PR

potiuk

Ah... I look at the checks and I was surprised, I have not seen Helm tests running. This

One more thing in selective checks (in breeze) we detect if Helm tests should be run:

        FileGroupForCi.HELM_FILES: [
            r"^chart",
            r"^airflow/kubernetes",
            r"^tests/kubernetes",
            r"^tests/charts",
        ],

amoghrajesh · 2023-07-06T11:28:32Z

Yeah I have made the changes there right: https://github.com/apache/airflow/pull/32394/files#diff-c6f469502ccc758f597b999e8ab5aebeba2ae82fb6dc8166e7d8a25969bcf3faR129

potiuk · 2023-07-06T11:29:10Z

There is one unnerving thing here as well - for some reason (and I have no idea why yet) I cannot see the "Unit tests: Helm" running in this PR. I will take a look after you fix all the static checks (and please rebase to latest main as well).

amoghrajesh · 2023-07-06T11:30:53Z

Let me rebase and let the CI run once. I am not sure what is failing in the static checks. It is not so clear from logs

potiuk · 2023-07-06T11:32:29Z

Not sure what the failure is. doesn't look related to the changes in this PR

Look at the output of it. It says exactly what it is - and prints you the command tat you should run to fix it. It's about changed breeze commmands that need regeneration of embedded Breeze images.

amoghrajesh · 2023-07-06T11:33:56Z

Thought that was a false alarm. Checking it

potiuk · 2023-07-06T11:34:18Z

potiuk · 2023-07-06T11:51:25Z

Ah.. Looking at the generated image, there is one more problem:

https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/global_constants.py#L123

This is why you had this "static check" failures. The image generation should not be needed, but there is one more place where "tests" / "charts" was used and now the list of allowed packages is wrong.

potiuk · 2023-07-06T11:51:59Z

You can switch to "swipe" view for the image generated and you will see what happened.

potiuk · 2023-07-06T11:54:05Z

BTW. It would also be great to filter out the __pycache__ folder from https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/global_constants.py#L123 - for some reason you had it in your environment, and we should filter it out then.

potiuk · 2023-07-06T13:15:21Z

They are failing because of the __pycache__. Look at --helm-test-package help output:

potiuk · 2023-07-06T13:16:25Z

For some reason, in your environment there is __pycache__ folder in the "helm-tests" folder - just filter it out in the place where it is generated (that's the lines I pointed out to you)

amoghrajesh · 2023-07-06T13:24:41Z

Oh, thanks for explaining so patiently. I could not see the folder because it was a hidden one! Silly me

potiuk · 2023-07-06T14:21:25Z

Now.... Coming back to unit tests that faik .. There seems to be coming from that chang. For example the first one determines the folder where chart is - this way:

chart_dir = Path(__file__).parents[3] / "chart"

And after the tests were moved from the "tests/charts" to "helm-tests" - this is no longer working - because instead of looking for chart in "/opt/airflow/chart", it looks in "/opt/chart".

So I think you need to also take a look at those tests that are failling and fix similar cases.

potiuk · 2023-07-06T14:23:34Z

BTW. And yeah, those are all (except pycache problems I kind of expected to see in this PR, so this is not really strange or unexpected :)

potiuk · 2023-07-06T14:29:14Z

Oh, thanks for explaining so patiently. I could not see the folder because it was a hidden one! Silly me

Not a problem, I think there is a value in doing it, as you will learn quite a bit more on how things work in Airflow CI/Breeze/tests etc. - I know well things that are obvious to me as I worked on it for quite some time, aren't necessary obvious to others. I think the important thing is that YOU do not loose the patience, and will go through all that with open approach accepting that there are completely new things coming up that you have to learn about and where this "simple" PR will take quite some time to complete, and that things will be looking wrong at times. There are quite a few moving pieces here that make the whole system works - the fact that you have breeze where you can run it all manually, local venv, but also CI that uses breeze and reproducible enviroment make it much more complex (but not complicated) I think. Complex in the sense that there are multiple pieces of puzzle that work together and each has a role, but every single one is simple (once you know the role, and know that it is there at all).

amoghrajesh · 2023-07-06T16:05:50Z

Thanks for the detailed mentoring, @potiuk
You have been very helpful and now I know quite some areas confidently in airflow :)

amoghrajesh · 2023-07-06T16:08:04Z

Hopefully we should get a green CI now. I have run the tests in the whole module. All looks good 👍🏽
Will also rebase to main so that any unknowns don't show up.

potiuk · 2023-07-06T16:17:27Z

🤞

potiuk · 2023-07-06T16:48:50Z

1 test left it seems :)

amoghrajesh · 2023-07-06T16:53:49Z

@potiuk I think we are done with it now. Hopefully this time it will :)

amoghrajesh · 2023-07-06T18:55:38Z

Clean build finally, @potiuk 🙂

potiuk · 2023-07-06T19:41:35Z

helm_tests/airflow_aux/test_pod_template_file.py

 @pytest.fixture(scope="class", autouse=True)
 def isolate_chart(request):
-    chart_dir = Path(__file__).parents[3] / "chart"
+    chart_dir = Path(__file__).parents[3] / "airflow" / "chart"


Suggested change

chart_dir = Path(__file__).parents[3] / "airflow" / "chart"

chart_dir = Path(__file__).parents[2] / "chart"

This is how it should be. Having "airflow" hard-coded, you assume that that Airflow has been checked out as "airflow" directory, which does not necessarily needs to be true. This would cause subtle problems that could be very difficult to fix when you will be running the tests in loca environment.

You can easily checkout airflow with a different "folder" name or even create a work-tree if you work on different branches in the same directory where you keep several different versions of airflow checked out.

For example I have:

code | - airflow -> here I mostly work on main | - airflow-copy -> here I mostly work on v2-6-test branch or similar

The airflow-copy is a worktree of the "airflow" repoository - which means that I have one git repository but I can have different branches of the same repo checked out in different folders.

If you do parents[3] / "airflow" / "chart", what will happen is that when I run tests in "airflow-copy", it will copy chart from the "airflow" folder, which might be an old tag or branch. That would be extremely difficult test failure to trace in those cases.

(Also in other places)

Great point. Thanks for the explanation. I will make the changes :)

potiuk

The "chart" directory deterrmining needs to be fixed (see comment)

amoghrajesh · 2023-07-07T10:08:23Z

@potiuk has any new hook been added to check for re2 library over re? The static checks fail with the error..

Use re2 over re....................................................................Failed
- hook id: check-usage-of-re2-over-re
- exit code: 1

helm_tests/airflow_aux/test_pod_template_file.py:19:import re
helm_tests/other/test_redis.py:19:import re
``

potiuk · 2023-07-07T14:18:32Z

🎉 🎉

…che#32394) --------- Co-authored-by: Amogh Desai <adesai@adesai-MBP16.local>

Migrating helm chart tests to test-charts to prevent test DB init

75b2cad

amoghrajesh requested review from ashb, jedcunningham and potiuk as code owners July 6, 2023 07:59

boring-cyborg bot added the area:dev-tools label Jul 6, 2023

potiuk reviewed Jul 6, 2023

View reviewed changes

potiuk requested changes Jul 6, 2023

View reviewed changes

review comments from potiuk

00422fa

amoghrajesh requested a review from potiuk July 6, 2023 09:34

removing remaining test-charts

64713fe

amoghrajesh changed the title ~~Migrating helm chart tests to test-charts to prevent test DB init~~ Migrating helm chart tests to helm_tests to prevent test DB init Jul 6, 2023

potiuk approved these changes Jul 6, 2023

View reviewed changes

potiuk requested changes Jul 6, 2023

View reviewed changes

Merge branch 'main' into moveTests

ef31c41

fixing static tests

3547e92

changing occurences of tests/charts

fd9179d

filtering out pycache

c530398

fixing chart_dir due to helm_tests change

ec4f0a1

Merge branch 'main' into moveTests

299e18c

fixing final unit tests

28a53cb

potiuk approved these changes Jul 6, 2023

View reviewed changes

potiuk reviewed Jul 6, 2023

View reviewed changes

potiuk requested changes Jul 6, 2023

View reviewed changes

review comments from potiuk

37bb216

amoghrajesh requested a review from potiuk July 7, 2023 05:46

Merge branch 'main' into moveTests

f0b2789

Merge branch 'main' into moveTests

5c41273

potiuk approved these changes Jul 7, 2023

View reviewed changes

potiuk merged commit 907aeeb into apache:main Jul 7, 2023

syedahsn pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Jul 11, 2023

Migrating helm chart tests to helm_tests to prevent test DB init (apa…

c292215

…che#32394) --------- Co-authored-by: Amogh Desai <adesai@adesai-MBP16.local>

ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Aug 2, 2023

ephraimbuddy added this to the Airflow 2.7.0 milestone Aug 2, 2023

	chart_dir = Path(__file__).parents[3] / "airflow" / "chart"
	chart_dir = Path(__file__).parents[2] / "chart"

Migrating helm chart tests to helm_tests to prevent test DB init #32394

Migrating helm chart tests to helm_tests to prevent test DB init #32394

Uh oh!

Conversation

amoghrajesh commented Jul 6, 2023

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

potiuk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amoghrajesh commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

amoghrajesh commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

amoghrajesh commented Jul 6, 2023

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

amoghrajesh commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

amoghrajesh commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

amoghrajesh commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amoghrajesh commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

amoghrajesh commented Jul 6, 2023

Uh oh!

amoghrajesh commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

potiuk commented Jul 6, 2023

Uh oh!

amoghrajesh commented Jul 6, 2023

Uh oh!

amoghrajesh commented Jul 6, 2023

Uh oh!

potiuk Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

amoghrajesh Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

potiuk left a comment •

edited

Loading

potiuk commented Jul 6, 2023 •

edited

Loading

potiuk commented Jul 6, 2023 •

edited

Loading

potiuk Jul 6, 2023 •

edited

Loading