I personally also think it's a good idea to remove caplog. I think I've also made the same remark in the past when I was asked to refactor mocked logs back to caplog in some PR (as this is/was the default way of working).
The advantage of caplog is that's easy to use, you don't have to wonder how it should be mocked (not that's that difficult), maybe we should think of some fixture for logging (and make it available in tests_common) which would make it as "easy" to use as caplog. -----Original Message----- From: Jarek Potiuk <ja...@potiuk.com> Sent: Thursday, 6 February 2025 20:44 To: dev@airflow.apache.org Subject: Re: [LAZY CONSENSUS] Remove caplog usage from Unit Tests EXTERNAL MAIL: Indien je de afzender van deze e-mail niet kent en deze niet vertrouwt, klik niet op een link of open geen bijlages. Bij twijfel, stuur deze e-mail als bijlage naar ab...@infrabel.be<mailto:ab...@infrabel.be>. If you have other idea how to avoid such problems in the future with caplog (other than careful review all the tests that can potentially modify loggers), I am all ears. On Thu, Feb 6, 2025 at 8:41 PM Jarek Potiuk <ja...@potiuk.com> wrote: > BTW. Ash - good example of why we want to get rid of caplog is here: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith > ub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F36807 > 274106&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cfaab0723557f4a82658d > 08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0%7C0%7C638744679276 > 105406%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDA > wMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sda > ta=IDhy0N733zok%2FWn95OmIkT5ivzLxuyC%2B42fSbByX7zk%3D&reserved=0 > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2576&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276118835%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=V%2BQLK1sIySAl4jZGwZd4vEMuncjqOvOF5eX3MHg9PiE%3D > &reserved=0>_______ > TestWasbHook.test_delete_container_generic_exception[ValueError] > _______ > 2560 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2577&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276132151%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=qmc58WneCuNdJI5U0X6B2AFK2yqtCwtKL9keGgkyoGQ%3D&r > eserved=0> > providers/tests/microsoft/azure/hooks/test_wasb.py:539: in > test_delete_container_generic_exception > 2561 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2578&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276148088%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=ucLZyNU4kwnNuDl%2FW0C7ovFZJ%2FQbQUZ%2Bwt3dQT1uFN > I%3D&reserved=0> assert "Error deleting container: mycontainer" in > caplog.text > 2562 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2579&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276156297%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=6%2FE79ZXOvRz0TKg2aX7wX86AXR6loqLSLaBwhZbVzI8%3D > &reserved=0>E > AssertionError: assert 'Error deleting container: mycontainer' in '' > 2563 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2580&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276164025%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=yCV3SPiq2jJzMz9IJDOnY2Gu5jrash%2Bw1DDNYmzBgLM%3D > &reserved=0>E > + where '' = <_pytest.logging.LogCaptureFixture object at > 0x7fa7109dcbe0>.text > 2564 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2581&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276171577%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=2OyK9Rk%2FFnoWyCz0bCO1zKnnKXL8wxwccxYpYzHnz8s%3D > &reserved=0>______ > TestWasbHook.test_delete_container_generic_exception[RuntimeError] > ______ > 2565 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2582&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276179212%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=crvJ9DHdyzCow3G%2BUUYyUb0uzN7%2BPFm0FBSMP8YaZ%2B > 4%3D&reserved=0> > providers/tests/microsoft/azure/hooks/test_wasb.py:539: in > test_delete_container_generic_exception > 2566 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2583&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276186710%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=D25j8wy3fWR0HGO3HjaPC9JMHuUh0Bg0zU1hX3H9c%2Bw%3D > &reserved=0> assert "Error deleting container: mycontainer" in > caplog.text > 2567 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2584&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276194214%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=nJyR3S9gjopsCvWbIPoC57o9FoadH4Fp3KXAA1jwbVI%3D&r > eserved=0>E > AssertionError: assert 'Error deleting container: mycontainer' in '' > 2568 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2585&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276201732%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=2t1RO9TYv8BZH5FCTczOHuw7AoiFlL9fecTH0rXTHBs%3D&r > eserved=0>E > + where '' = <_pytest.logging.LogCaptureFixture object at > 0x7fa710a65040>.text > 2569 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2586&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276209192%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=Q7bLNYi9QjuBpJF7GByk7Z%2FwMkp2k7bYzgSn9afOswU%3D > &reserved=0>____________ > TestWasbHook.test_delete_container_resource_not_found _____________ > 2570 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2587&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276216680%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=ZPPi85voOn200mX%2FVnJsYHTeh4LIlKu0%2B4RqkYugTpo% > 3D&reserved=0> > providers/tests/microsoft/azure/hooks/test_wasb.py:548: in > test_delete_container_resource_not_found > 2571 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2588&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276224168%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=GQM64dxw00ogiaHhRmXdJO37s8aBNJ3hkRwh0OU1dDE%3D&r > eserved=0> assert "Unable to delete container mycontainer (not found)" > in caplog.text > 2572 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2589&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276231580%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=rhvIomYgWHpjiY9262IUnNaNPGiHoAjLxh3yuRWiKrQ%3D&r > eserved=0>E > AssertionError: assert 'Unable to delete container mycontainer (not found)' > in '' > 2573 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Fapache%2Fairflow%2Factions%2Fruns%2F13185566641%2Fjob%2F3680 > 7274106%23step%3A6%3A2590&data=05%7C02%7Cdavid.blain%40infrabel.be%7Cf > aab0723557f4a82658d08dd46e6cbb1%7Cb82bc314ab8e4d6fb18946f02e1f27f2%7C0 > %7C0%7C638744679276239029%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy > dWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D% > 3D%7C0%7C%7C%7C&sdata=c7Zayr9xUPIwNO6whH2b5l1dtCRfNUbLz5WoGX2i7FA%3D&r > eserved=0>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 >>> >>>
