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://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 >>> >>>
