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
* 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 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 > >>> > >>> > >
