Cool. I'd love to have the caplog issue fixed without removing caplog convenience. Let's see if we can get the missing 20% confidence back to make sure no more time of contributors and maintainers is lost because of this one
On Sat, Feb 15, 2025 at 10:35 PM Ash Berlin-Taylor <a...@apache.org> wrote: > > > @pytest.fixture(name="caplog") > def override_caplog(request): > fixture = pytest.LogCaptureFixture(request.node) > yield fixture > fixture._finalize() > > if "airflow.logging_config" in sys.modules: > import airflow.logging_config > > airflow.logging_config.configure_logging() > > > I am 80% confident that this is going to fix the flakeyness from caplog. > I’m introducing it in my Trigger re-work PR. > > > On 7 Feb 2025, at 13:17, Jarek Potiuk <ja...@potiuk.com> wrote: > > > > As clearly explained in the above example - this is not a problem with > > databricsk and azure. This is aproblem in Azure BECAUSE we move > Databricks. > > So clearly a side effect of one group of tests on another. As such - we > > have no idea how many other cases we have like that and who might be > > causing it. We only know that a lot of time of people who try to keep the > > main green is lost because of that. > > > >> This is not very collaborative and accepting that people have a life > > outside of their computer’s Jarek. > > > > No. It's collaborative. This is how our social contract works. If you > want > > to change the rules and have good reasoning for that - propose a change. > > And you still can propose to revert the decision in a separate thread. > This > > happened in the past and it's perfectly fine. > > > > We cannot completely change the social agreement we have on decision > making > > work in our community because one person missed a thread on a really > > insignificant thing. We should just accept it and follow, or propose to > > change a contract. That's how a community works. > > > > > > J > > > > > > > > On Fri, Feb 7, 2025 at 2:05 PM Jarek Potiuk <ja...@potiuk.com> wrote: > > > >>> How is it flakey though? This is the part I’m asking for explaining > >> about. Surely it’s messed up and _ALWAYS_ messed up? > >> > >> See above example posted in the thread. It explains everything. I am not > >> sure if you missed it, but it's the third time I am explaining it and > you > >> keep on asking. The example I have shows not only the copy & paste of > the > >> problem, but link to the exact failure. > >> > >> On Fri, Feb 7, 2025 at 2:00 PM Ash Berlin-Taylor <a...@apache.org> > wrote: > >> > >>> How does caplog introduce flakiness though? That still isn’t > >>> > >>> > >>>> On 7 Feb 2025, at 11:07, Jarek Potiuk <ja...@potiuk.com> wrote: > >>>> > >>>> Ash - take a close look at the example I sent. Maybe you missed it. > >>>> > >>>> * Caplog gets logs from loggers - If your loggers are messed 1000 > tests > >>>> before and not restored - you will have failing tests. Side effects > >>> > >>> How is it flakey though? This is the part I’m asking for explaining > >>> about. Surely it’s messed up and _ALWAYS_ messed up? > >>> > >>>> > >>>> * Mocking logs is mocking lag method call. It always work and is > >>>> side-effect resistant > >>>> > >>>> > >>>> I would only agree to restore back Caplog usag (which as I mentioned > >>>> requires new proposal, new consensus or voting as the consensus is > >>> already > >>>> reached). I will expect the person proposing it to track all the cases > >>>> where side effects have been causing problems > >>> > >>> This is not very collaborative and accepting that people have a life > >>> outside of their computer’s Jarek. > >>> > >>> > >>> Here’s a proposal for you: don’t create needless code churn and > wholesale > >>> replace caplog, instead only swap if it’s not actually a problem. > Replace > >>> it only when it is noticed as being flakey - which you’ve pointed out > two > >>> cases, Databricks and Azure.. We have 287 instances of `caplog` in our > >>> tests. How many are actually flakey? Why change things that aren’t > broken? > >>> > >>>> > >>>> > >>>> This happened (and let me repeat it again - I hope it will not be > again > >>>> ignored this time). About 10 to 15 times in the last few weeks. > >>>> > >>>> Ash - if you want to propose restoring Caplog usage, please start a > new > >>>> thread. Ideally with explanation why. > >>>> > >>>> J. > >>>> > >>>> pt., 7 lut 2025, 10:15 użytkownik Ash Berlin-Taylor <a...@apache.org> > >>>> napisał: > >>>> > >>>>> How does replacing caplog with mocking help anything, assuming that > you > >>>>> test what is logged against the mock? > >>>>> > >>>>> Please explain it to me like I’m 5, cos I don’t see how it makes the > >>>>> blindest bit of difference. Testing what is on the caplog output vs > >>> what is > >>>>> recorded on the mock log object is functional identical. If caplog > >>> tests > >>>>> are flakey then mock log tests will be equally flakey. The caplog > >>> process > >>>>> itself is not flakey. It’s what is producing the log that is flakey, > >>> no? > >>>>> > >>>>> If you are talking about removing logging tests entirely if they > aren’t > >>>>> providing value: sure, but that isn’t what I understood this to be > >>> about. > >>>>> > >>>>>> On 6 Feb 2025, at 19:41, Jarek Potiuk <ja...@potiuk.com> wrote: > >>>>>> > >>>>>> BTW. Ash - good example of why we want to get rid of caplog is > here: > >>>>>> > >>>>>> > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106 > >>>>>> > >>>>>> This is PR to move databricks provider to the new provider > structure. > >>>>>> Suddenly in that PR ***WASB*** (yes - we moved **DATABRICKS**) > tests > >>> are > >>>>>> failing: > >>>>>> > >>>>>> > >>>>>> =================================== FAILURES > >>>>>> =================================== > >>>>>> 2559 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2576 > >>>>>> _______ > >>>>>> TestWasbHook.test_delete_container_generic_exception[ValueError] > >>> _______ > >>>>>> 2560 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2577 > >>>>>> > >>>>>> providers/tests/microsoft/azure/hooks/test_wasb.py:539: in > >>>>>> test_delete_container_generic_exception > >>>>>> 2561 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2578 > >>>>>> > >>>>>> assert "Error deleting container: mycontainer" in caplog.text > >>>>>> 2562 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2579 > >>>>>> E > >>>>>> AssertionError: assert 'Error deleting container: mycontainer' in '' > >>>>>> 2563 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2580 > >>>>>> E > >>>>>> + where '' = <_pytest.logging.LogCaptureFixture object at > >>>>>> 0x7fa7109dcbe0>.text > >>>>>> 2564 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2581 > >>>>>> ______ > >>>>>> TestWasbHook.test_delete_container_generic_exception[RuntimeError] > >>> ______ > >>>>>> 2565 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2582 > >>>>>> > >>>>>> providers/tests/microsoft/azure/hooks/test_wasb.py:539: in > >>>>>> test_delete_container_generic_exception > >>>>>> 2566 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2583 > >>>>>> > >>>>>> assert "Error deleting container: mycontainer" in caplog.text > >>>>>> 2567 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2584 > >>>>>> E > >>>>>> AssertionError: assert 'Error deleting container: mycontainer' in '' > >>>>>> 2568 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2585 > >>>>>> E > >>>>>> + where '' = <_pytest.logging.LogCaptureFixture object at > >>>>>> 0x7fa710a65040>.text > >>>>>> 2569 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2586 > >>>>>> ____________ > >>>>>> TestWasbHook.test_delete_container_resource_not_found _____________ > >>>>>> 2570 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2587 > >>>>>> > >>>>>> providers/tests/microsoft/azure/hooks/test_wasb.py:548: in > >>>>>> test_delete_container_resource_not_found > >>>>>> 2571 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2588 > >>>>>> > >>>>>> assert "Unable to delete container mycontainer (not found)" in > >>>>> caplog.text > >>>>>> 2572 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2589 > >>>>>> E > >>>>>> AssertionError: assert 'Unable to delete container mycontainer (not > >>>>> found)' > >>>>>> in '' > >>>>>> 2573 > >>>>>> < > >>>>> > >>> > https://github.com/apache/airflow/actions/runs/13185566641/job/36807274106#step:6:2590 > >>>>>> E > >>>>>> + where '' = <_pytest.logging.LogCaptureFixture object at > >>>>>> 0x7fa710c37bb0>.text > >>>>>> > >>>>>> > >>>>>> And those are the failing tests. They are using caplog and I am not > >>> sure > >>>>> if > >>>>>> you can figure out why suddenly the expected log entries cannot be > >>> found > >>>>> in > >>>>>> caplog. > >>>>>> > >>>>>> My best guess is that somewhere, some tests (databricks) are > >>>>> reconfiguring > >>>>>> the loggers, and it messes up with caplog. Classic side effects. > >>>>>> > >>>>>> Mocking log methods directly avoids side-effects involved with > logger > >>>>>> reconfiguraton. This is about 10th time we have this problem while > >>>>> moving > >>>>>> providers. > >>>>>> Here are the tests that are failing. > >>>>>> > >>>>>> @pytest.mark.parametrize("exc", [ValueError, RuntimeError]) > >>>>>> def test_delete_container_generic_exception(self, exc: > >>>>> type[Exception], > >>>>>> caplog): > >>>>>> hook = WasbHook(wasb_conn_id=self.azure_shared_key_test) > >>>>>> with mock.patch.object(WasbHook, "_get_container_client") as m: > >>>>>> m.return_value.delete_container.side_effect = > >>>>>> exc("FakeException") > >>>>>> caplog.clear() > >>>>>> caplog.set_level("ERROR") > >>>>>> with pytest.raises(exc, match="FakeException"): > >>>>>> hook.delete_container("mycontainer") > >>>>>> assert "Error deleting container: mycontainer" in > >>> caplog.text > >>>>>> > >>>>>> def test_delete_container_resource_not_found(self, caplog): > >>>>>> hook = WasbHook(wasb_conn_id=self.azure_shared_key_test) > >>>>>> with mock.patch.object(WasbHook, "_get_container_client") as m: > >>>>>> m.return_value.delete_container.side_effect = > >>>>>> ResourceNotFoundError("FakeException") > >>>>>> caplog.clear() > >>>>>> caplog.set_level("WARNING") > >>>>>> hook.delete_container("mycontainer") > >>>>>> assert "Unable to delete container mycontainer (not found)" > >>> in > >>>>>> caplog.text > >>>>>> > >>>>>> @mock.patch.object(WasbHook, "delete_blobs") > >>>>>> def test_delete_single_blob(self, delete_blobs, > >>>>>> mocked_blob_service_client): > >>>>>> hook = WasbHook(wasb_conn_id=self.azure_shared_key_test) > >>>>>> hook.delete_file("container", "blob", is_prefix=False) > >>>>>> delete_blobs.assert_called_once_with("container", "blob") > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>> On Thu, Feb 6, 2025 at 3:56 PM Jarek Potiuk <ja...@potiuk.com> > wrote: > >>>>>> > >>>>>>> Just - note that the 72 hrs are specifically designed to cover the > >>>>>>> weekend. So when there are such - rather insignificant - issues, I > >>>>> think we > >>>>>>> should be ok. We have had many of our votes - including the > >>> absolutely > >>>>> most > >>>>>>> important - releases - lasting 72 hours over weekend and it has > never > >>>>> been > >>>>>>> a problem, we only extended it including weekends where the matters > >>> were > >>>>>>> really important and we expected them to be contentious (this one > was > >>>>> not). > >>>>>>> > >>>>>>> Does it mean Ash that we propose extending all kinds of consensus > and > >>>>>>> voting in this way ? It's the first time it has been raised, so > maybe > >>>>>>> that's a good idea to make it longer now when we are all > additionally > >>>>> busy > >>>>>>> with Airflow 3, but I don't see any reason why it should be a > problem > >>>>> "in > >>>>>>> general". WDYT? > >>>>>>> > >>>>>>> Note that your objection is also 5 days after the initial message, > so > >>>>> even > >>>>>>> if it would be 2 days more, that would not change anything. > >>>>>>> > >>>>>>> IMHO the consensus is reached. and If you want to revert that you > >>> should > >>>>>>> start a new thread Ash. > >>>>>>> > >>>>>>> J. > >>>>>>> > >>>>>>> On Thu, Feb 6, 2025 at 3:35 PM Ash Berlin-Taylor <a...@apache.org> > >>>>> wrote: > >>>>>>> > >>>>>>>> Please be aware of time when you are proposing this. 48 out of > the > >>> 72 > >>>>>>>> hours your had here were over the weekend which doesn’t give many > >>>>> people > >>>>>>>> adequate time to read and think upon this. > >>>>>>>> > >>>>>>>> > >>>>>>>> Because I’m -1 on this as I don’t have enough info (see my other > >>> email) > >>>>>>>> to understand why this change makes the first bit of difference. > >>>>>>>> > >>>>>>>>> On 4 Feb 2025, at 22:38, Buğra Öztürk <ozturkbugr...@gmail.com> > >>>>> wrote: > >>>>>>>>> > >>>>>>>>> Hey all, > >>>>>>>>> > >>>>>>>>> The consensus has been reached. > >>>>>>>>> > >>>>>>>>> Regards, > >>>>>>>>> > >>>>>>>>> On Sat, Feb 1, 2025 at 5:38 PM Buğra Öztürk < > >>> ozturkbugr...@gmail.com> > >>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> Hello everyone, > >>>>>>>>>> > >>>>>>>>>> There have been recurring discussions about minimizing the usage > >>> of > >>>>>>>> caplog > >>>>>>>>>> in unit tests. If logs are necessary for a test, we should mock > >>> them > >>>>>>>> unless > >>>>>>>>>> the test strictly requires actual log output. Otherwise, the > >>> caplog > >>>>>>>> tests > >>>>>>>>>> should be removed and logs should be mocked. > >>>>>>>>>> > >>>>>>>>>> During recent large migrations, changes and ongoing CI/CD > efforts, > >>>>>>>> caplog > >>>>>>>>>> has proven to be flaky in unit tests. They frequently cause red > >>>>>>>> pipelines > >>>>>>>>>> in PRs and scheduled CI/CD runs. There have been local > >>> discussions on > >>>>>>>>>> removing caplog from unit tests. To formalize this, I propose a > >>> lazy > >>>>>>>>>> consensus to remove and prevent caplog usage in unit tests, > >>> ensuring > >>>>>>>> logs > >>>>>>>>>> are mocked when needed and disallowing caplog without mocking > >>> unless > >>>>>>>>>> explicitly approved. > >>>>>>>>>> > >>>>>>>>>> *Why should caplog be avoided?* > >>>>>>>>>> > >>>>>>>>>> - Big maintenance effort on CI/CD > >>>>>>>>>> - Instability > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> *What should we do instead if we use caplog in a unit test?* > >>>>>>>>>> > >>>>>>>>>> - Mocking the Log > >>>>>>>>>> > >>>>>>>>>> *In the Scope of this consensus* > >>>>>>>>>> > >>>>>>>>>> - Remove caplog usage from unit tests if possible > >>>>>>>>>> - Mock logs and remove caplog from unit tests if possible > >>>>>>>>>> - Exceptional cases will be subject to maintainer approval > >>>>>>>>>> - Prevent caplog to be included in unit tests without explicitly > >>>>>>>>>> mocking the log > >>>>>>>>>> > >>>>>>>>>> *Action Items related to the above Scope:* > >>>>>>>>>> > >>>>>>>>>> - Scan and replace caplog tests with mocking where possible > >>>>>>>>>> - Scan and remove caplog tests where possible > >>>>>>>>>> - Include CI check to prevent adding additional caplog tests > >>> without > >>>>>>>>>> Mocking and/or without approval from a maintainer to allow > >>>>>>>> flexibility in > >>>>>>>>>> some exceptional cases > >>>>>>>>>> - Create documentation with examples for implementing tests > using > >>>>>>>> logs > >>>>>>>>>> without caplog (e.g., using mocking or avoiding logs entirely) > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> The lazy consensus will be reached on 2025-02-04, (unless > someone > >>>>>>>> surfaces > >>>>>>>>>> an objection). > >>>>>>>>>> -- > >>>>>>>>>> Bugra Ozturk > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> -- > >>>>>>>>> Bugra Ozturk > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>> --------------------------------------------------------------------- > >>>>>>>> To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > >>>>>>>> For additional commands, e-mail: dev-h...@airflow.apache.org > >>>>>>>> > >>>>>>>> > >>>>> > >>>>> > >>> > >>> > >>> --------------------------------------------------------------------- > >>> To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > >>> For additional commands, e-mail: dev-h...@airflow.apache.org > >>> > >>> > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > For additional commands, e-mail: dev-h...@airflow.apache.org > >
