Clear DB before individual module tests

[Taragolis]

Follow-up: #28337 (comment)

Some hacks which uses in PR

1. module scope with parsing "provider name" instead of package scope. pytest decide that package is airflow and run only once
2. Use separate clear tests helpers instead of airflow.utils.db.resetdb(). locally cause some errors with duplicate constraint.

[eladkal]

Should/can we generate this list automatically?
Hard coding has risk that we will forget to update the list when needed

[potiuk]

We can check for presence of provider.yaml. Or better - rely on generated/provider_dependencies.json instead. the provider_dependencies.json is guaranteed to be updated by pre-commit and it is far easier to use than scanning folders, as well as it's intention is to remain stable when we refactor and move the providers.

[potiuk]

Another comment here @Taragolis . Maybe we do not need that at all. Don't we want to do it for ALL packages that are "leaves" - not only for providers (including all airflow packages)? I think that would be a nice compromise between isolation and speed of execution

[potiuk]

There is also one side-effect that we should take into account here. This is fine when we run the tests in CI, but the way it is implemented now, it will also run the auto-fixture even if you run a single unit test. This will dramatically slow-down any iterations you do I am afraid (and it does not really matter if we do it only for Providers or for whole airflow).

I think this can be solved by only doing this when we run:

1. for the first time ever (so some kind of check if db is initialized)
2. or always when we run in CI (CI env variable set to "true").

The 1) would be nice for first-time users - we badly miss it now, the user needs to know that they have to run the tests with --with-db-init flag.

[Taragolis]

I think we could . Just need to scan directory until found one of the folder: hooks, operators, sensors

[Taragolis]

I could check also request fixture tomorrow, I thought it should contain session information, such as how many tests collected and if it equal (or less, I don't know how it possible) 1 then do not cleanup anything.

[Taragolis]

And also remove yield in this fixture we do not need any teardown, at least yet

[potiuk]

I could check also request fixture tomorrow, I thought it should contain session information, such as how many tests collected and if it equal (or less, I don't know how it possible) 1 then do not cleanup anything.

I think this is not really needed now - the overhead with using the test_utils classes is really low indeed. I believe we should enable it for all tests and compare execution time on CI and if it is < 5% overhead in total or so, we can leave it on for all modules.

[Taragolis]

For long term we also need to consider get rid of most of the stuff in test.test_utils and replace them by equivalent as fixture/context manager/markers but this better complete after migrate remaining tests to pytest and document them.

Because most of the new contributors just copy-paste some stuff from one test to another without any idea how it actually work.

[potiuk]

For long term we also need to consider get rid of most of the stuff in test.test_utils and replace them by equivalent as fixture/context manager/markers but this better complete after migrate remaining tests to pytest and document them.

Because most of the new contributors just copy-paste some stuff from one test to another without any idea how it actually work.

Yep. Full agreement here.

[uranusjr]

Would it be a good idea to also clean after the provider tests? I feel like it’s generally a good principle for one thing to clean up one’s own stuffs. We could do

1. Clean when entering the providers directory.
2. Clean after each provider is finished.

[potiuk]

I tihnk we've came to proposal that with such (low) overhead we have we can clean up database not before/after every provider but before (after?) any module. As you can see from the attached videos above - overall overhead for running single test is neglectible if we do (~0.2 s is almost not noticeable). It will be far less if we run the whole group of tests.

If we always clean before each module - this will work not only for providers, but also for regular airflow tests - with such a low overhead, we do not need to do cleanup because we have 100% guarantee, that the DB will be cleaned before runnin any test.

[potiuk]

We still will need to see how much it will impact the overall test time, but I am cautiously optimistic it won't be a lot. And we can move it up to package as well if we find module is too much.

And this approach has really some interesting properties if we do such cleanup unconditionally before every module:

1. We can stop worrying about any DB side-effects outside of the modules. We have all but guarantee that any side-effects will not cross the module boundaries

2. Running all tests in a module is usually what people do - because it is easy. And whenever we see a suspicious side-effect, it will be super-easy to reproduce - just run the whole module - it will run identically in CI and locally because each module is isolated.

3. When a new user checks out and run tests, they will not have to think about initializing the database - similarly when the database has changed. So far we had --with-db-init switch to let airflow reset the unit test DB but it was pretty brittle and not many people even knew about its existence. We will be able to remove that now - because every time you run a test you have guarantee to get a fresh DB with the current structure.

[potiuk]

Another con of cleaning after the test - this is rarely used, but potentially not cleaning the DB after the test gives the developer the luxury that we can easily inspect the DB after the test completes - so for running individual tests, I think leaving the DB intact after the test has some nice debugging property as well.

[Taragolis]

I've better keep --with-db-init time to time devs still need to clear DB (something changed in structure).

I have for this run configurations, but someone could find --with-db-init more useful rather then this

[potiuk]

Yes. We should leave it in.

[Taragolis]

Ok, lets have a look.

I change it and make it as module scoped autouse fixture

[Taragolis]

Also add option which disable auto cleanup db. Of course it is not affect explicit clear parts

[Taragolis]

It is debugging time!

[Taragolis]

Fingers cross. Locally it run without any issues

[potiuk]

Nice!

[potiuk]

Cool. This is indeed important, though many tests will not work anyway with xdist.

[potiuk]

I like the escalation here and informative message.

[potiuk]

I guess this is accidental fix that was needed? Can we maybe separate it out to another PR (just for trackability)?

[Taragolis]

I move this usage into class fixture.

    def _set_attrs(self, app_for_kerberos, dagbag_to_db):

[potiuk]

I see. Right. Should be maybe change the name of the class fixture? to initialize or something?
I was under the impressuion (due to _set_attrs name) it was doing something different - and now it is not really setting the attributes.

[Taragolis]

Yeah. This is a good point.

BTW we have quite a few tests classes where we use method (function) scoped fixture. And most of them has different name, so might be also a good point to make them more or less consistent.

[potiuk]

❤️ it this way. Few nits for possible separation of some loosely related changes into separate PRs.

[potiuk]

BTW. I do not see a noticeable impact on the times of execution of the tests ... Which is cool if this is really the case. Are we sure it works :) ?

[Taragolis]

@potiuk I removed all optional changes and kept only related to this PR: autofixture and changes in API tests + Kerberos Integration

Also found that if we call DAG.sync_to_db() it no has any affect to database, so I change it to same method in DagBag object, I do not know is it expected or it is some kind of bug.

Example of changes

--- a/tests/api/client/test_local_client.py
+++ b/tests/api/client/test_local_client.py
@@ -44,7 +44,7 @@
 class TestLocalClient:
     @classmethod
     def setup_class(cls):
-        DagBag(example_bash_operator.__file__).get_dag("example_bash_operator").sync_to_db()
+        DagBag(example_bash_operator.__file__, include_examples=False).sync_to_db()
 
     def setup_method(self):
         clear_db_pools()

Previously this part work because we do not clear this part of metadatabase in API tests and tests use initial database state.

[ephraimbuddy]

Why set include_examples=True? Won't it cost some more time?

[Taragolis]

Some API tests use example DAGs: example_subdag_operator, example_trigger_target_dag and others.
Before changes in this PR all example serialised DAGs stored into database so we did not have issues with it but after we start cleanup DB before each module we have.

I'd rather to say this is temporary solution as target we need to use DAGs from:

• tests/dags
• tests/dags_corrupted
• tests/dags_with_system_exit

[potiuk]

Yeah. Those changes are needed .

[potiuk]

Also found that if we call DAG.sync_to_db() it no has any affect to database, so I change it to same method in DagBag object, I do not know is it expected or it is some kind of bug.

Not sure about it. But I do not see it to have any noticeable effect on time of running the tests, so let's keep it this way.

[Taragolis]

Yeah. Those changes are needed .

Echo from the past. I trace back to initial commit from 2017 🤣

[potiuk]

I am good with how it looks like now :)

[Taragolis]

Let's check with public runners

[potiuk]

Looks GOOOD. Shall we ?

[Taragolis]

Yeah. Let's merge.