Personally I dislike things like assert_called_once_with etc. since they are easy to miss when you scan a test to see what they are trying to check. An 'assert' keyword stands out (it’s always the first word in the line), especially with syntax highlighting.
I do agree the proposed Pytest style is arguably less readable. I offer another syntax. from unittest.mock import call assert mock_http_run.mock_calls == [ call( endpoint=f"api/v2/accounts/{expected_account_id}/", data=None, extra_options=None, ) ] assert mock_paginate.mock_calls == [] To me, this is on par with assert_called_once_with et al. in terms of readability, and arguably better for test authors since you don’t need to remember the various function names anymore, but only 'mock_calls' and the 'call' helper class. TP > On Jul 9, 2025, at 23:28, Ferruzzi, Dennis <ferru...@amazon.com.INVALID> > wrote: > > I'm a bit late to the party, and really only reiterating what has already > been said, but of the two examples (original and your rewrite, I prefer the > original. I think as a general rule, we tend to use the assert_called_once, > etc with mocks butt he asserts with non-mocked variables. > > I am all for more documentation, but I'd have a slight preference towards > keeping the existing structure. > > > - ferruzzi > > > ________________________________ > From: Kyungjun Lee <kyungjunlee...@gmail.com> > Sent: Tuesday, July 8, 2025 2:13 AM > To: dev@airflow.apache.org > Subject: RE: [EXT] [DISCUSS] Consistent test assertion style: pytest-native > vs unittest-style > > CAUTION: This email originated from outside of the organization. Do not click > links or open attachments unless you can confirm the sender and know the > content is safe. > > > > AVERTISSEMENT: Ce courrier électronique provient d’un expéditeur externe. Ne > cliquez sur aucun lien et n’ouvrez aucune pièce jointe si vous ne pouvez pas > confirmer l’identité de l’expéditeur et si vous n’êtes pas certain que le > contenu ne présente aucun risque. > > > > Thank you Ash and Amogh Desai for your insights and explanations. > The information you shared has been incredibly helpful and is contributing > a lot to my growth. > > 2025년 7월 8일 (화) 오후 2:54, Amogh Desai <amoghdesai....@gmail.com>님이 작성: > >> Agreed, I personally also find the current way to be easier to read and in >> most >> cases we want to assert if "something" was called, irrespective of the >> order it was >> called in. So the dedicated function based way works well for me. >> >> If I want to test a order, I'd rather call parts of code that I want to >> test explicitly and assert >> on them. >> >>> This happens because the mock object is not properly recognized as a mock >> instance at type-checking time, which might confuse some contributors, >> especially new ones relying on type hints or static analysis tools. >> >> Regarding this, I'd say you can either cast mocks to their types as one >> way: >> `mock_http_run: MagicMock = mock_http_run` -- give or take, or use >> `autospec` to make the mock reflect the signature of the object? Check out: >> https://docs.python.org/3/library/unittest.mock.html#autospeccing >> >> Thanks & Regards, >> Amogh Desai >> >> >> On Mon, Jul 7, 2025 at 6:13 PM Ash Berlin-Taylor <a...@apache.org> wrote: >> >>> def test_get_account(self, mock_paginate: Mock, mock_http_run: Mocj, >>> conn_id, account_id): >>> >>> >>> Might fix that? IDEs in general do not cope well with purest tests, and >>> are missing context on what most of the variables are, be it >> parameterised >>> values or fixture values, so this isn’t a problem that is unique to >> mocks. >>> >>>> On 7 Jul 2025, at 12:47, Kyungjun Lee <kyungjunlee...@gmail.com> >> wrote: >>>> >>>> I'd like to follow up on our previous discussion about pytest-native vs >>>> unittest-style assertions. >>>> >>>> While working on the following test case: >>>> >>>> ```python >>>> >>>> @pytest.mark.parametrize( >>>> argnames="conn_id, account_id", >>>> argvalues=[(ACCOUNT_ID_CONN, None), (NO_ACCOUNT_ID_CONN, >> ACCOUNT_ID)], >>>> ids=["default_account", "explicit_account"], >>>> ) >>>> @patch.object(DbtCloudHook, "run") >>>> @patch.object(DbtCloudHook, "_paginate") >>>> def test_get_account(self, mock_paginate, mock_http_run, conn_id, >>>> account_id): >>>> hook = DbtCloudHook(conn_id) >>>> hook.get_account(account_id=account_id) >>>> >>>> assert hook.method == "GET" >>>> >>>> _account_id = account_id or DEFAULT_ACCOUNT_ID >>>> hook.run.assert_called_once_with( >>>> endpoint=f"api/v2/accounts/{_account_id}/", data=None, >>>> extra_options=None >>>> ) >>>> hook._paginate.assert_not_called() >>>> >>>> ``` >>>> >>>> My IDE shows a warning: >>>> Cannot find reference 'assert_called_once_with' in 'function'. >>>> >>>> This happens because the mock object is not properly recognized as a >> mock >>>> instance at type-checking time, which might confuse some contributors, >>>> especially new ones relying on type hints or static analysis tools. >>>> >>>> I realized that this aspect of mock handling might be missing from our >>>> previous discussions. I wanted to bring it up as part of the broader >>>> conversation about test styles—particularly how we balance IDE/tooling >>>> support with readability and style consistency. >>>> >>>> Curious to hear your thoughts on this! >>>> >>>> @ash @potiuk >>>> >>>> 2025년 7월 6일 (일) 오후 8:09, Kyungjun Lee <kyungjunlee...@gmail.com>님이 작성: >>>> >>>>> Thank you @Potiuk for pointing out the intent behind the “one assert >> per >>>>> test” principle, and @ash for highlighting how using dedicated mock >>> assert >>>>> functions can make the code easier to read and understand. These were >>>>> perspectives I hadn’t fully considered, and I really appreciate you >>> sharing >>>>> them. >>>>> >>>>> Thanks to your input, I was able to read more materials and broaden my >>>>> thinking on the topic. That said, my original focus was more on the >> idea >>>>> that sticking to plain assert statements lowers the entry barrier for >>> new >>>>> contributors—because they don’t have to learn multiple assertion >> styles. >>>>> >>>>> Still, as @Potiuk mentioned, I’ll put more thought into making the >>> testing >>>>> guidelines clearer and more concrete—especially since that helps >>> AI-based >>>>> tools as well 😄 >>>>> >>>>> For reference, here’s one of the articles I read: >>>>> https://medium.com/@kentbeck_7670/test-desiderata-94150638a4b3 >>>>> >>>>> Thank you to everyone who took part in this discussion. >>>>> >>>>> 2025년 7월 6일 (일) 오후 3:42, Ash Berlin-Taylor <a...@apache.org>님이 작성: >>>>> >>>>>> I personally find the dedicated functions way easier to read the >> intent >>>>>> behind, it’s one function/statement vs 3. More so when you don’t care >>> about >>>>>> the order of calls, just that something was called (where to do this >>>>>> manually we’d need to reimplement the helper function) >>>>>> >>>>>> Additionally pytest already rewrites those to have nicer error >>> messages, >>>>>> but the dedicated mock assert finding are much easier to read the >> code >>> and >>>>>> understand the intent to me, so I’i am for staying with the dedicated >>>>>> assert functions >>>>>> >>>>>> -ash >>>>>> >>>>>>> On 5 Jul 2025, at 13:30, Kyungjun Lee <kyungjunlee...@gmail.com> >>> wrote: >>>>>>> >>>>>>> I’ve learned a lot of things I didn’t know before. >>>>>>> Thank you so much for all your help — I really appreciate it. >>>>>>> I’ll get started on this right away! >>>>>>> >>>>>>> 2025년 7월 5일 (토) 오후 9:18, Jarek Potiuk <ja...@potiuk.com>님이 작성: >>>>>>> >>>>>>>>> Haha ... I'm curious — which part sounded like ChatGPT style? >>>>>>>> English isn't my first language, so I did get a little help from >> it. >>>>>>>> >>>>>>>> That whole paragraph :) . >>>>>>>> >>>>>>>>>> Sure! Since you asked for how the test *should* be written, I >> took >>>>>> the >>>>>>>> opportunity to clean it up using a more pytest-native style while >>> also >>>>>>>> fixing the mock order issue. >>>>>>>> >>>>>>>> "Sure! Since you asked ..." sounds like an AI bot. >>>>>>>> >>>>>>>>> That got me thinking — what do you think about adding a guideline >>>>>>>> document >>>>>>>> for writing tests, similar to how we have a coding style guide? >>>>>>>> >>>>>>>> Absolutely - we already have some "seed' of it "Writing tests" >>>>>> chapter in >>>>>>>> contributing guideline, but we could definitely make it more >>> detailed. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>> >>> >> https://github.com/apache/airflow/blob/main/contributing-docs/testing/unit_tests.rst#writing-unit-tests >>>>>>>> >>>>>>>> And - speaking of AI - this is becoming more and more important to >>>>>> describe >>>>>>>> any common rules we have and context - so that using Agentic AI >>> yields >>>>>>>> better results. Kaxil already added >>>>>>>> https://github.com/apache/airflow/blob/main/AGENTS.md which >>> describes >>>>>>>> context for coding agents lile Codex - and we could improve it and >>> link >>>>>>>> more docs from our repo if they get more of our agreed >> "conventions" >>> - >>>>>> then >>>>>>>> Agents would get it as context and their generated code would be >>>>>> consistent >>>>>>>> with what we describe there. >>>>>>>> >>>>>>>> In a way - I think having a good documentation on processes, tools >>> and >>>>>>>> conventions was always something I've been after, but with the >>> Agentic >>>>>>>> workflows it might significantly boost the quality of generated >> code >>>>>> if we >>>>>>>> have more of those conventions and guidelines described. >>>>>>>> >>>>>>>> So .... ABSOLUTELY ... the more we describe in there, the better. >> And >>>>>> we >>>>>>>> have no more excuse that "anyhow no-one reads it" - because the >>> coding >>>>>>>> agents WILL be reading it and acting accordingly. So I think this >> is >>> a >>>>>> very >>>>>>>> good investment to make. >>>>>>>> >>>>>>>> J. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> On Sat, Jul 5, 2025 at 2:07 PM Kyungjun Lee < >>> kyungjunlee...@gmail.com >>>>>>> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>> Haha ... I'm curious — which part sounded like ChatGPT style? >>>>>>>>> English isn't my first language, so I did get a little help from >> it. >>>>>>>>> >>>>>>>>> But thank you — I actually learned something new from your >> comment! >>>>>>>>> That got me thinking — what do you think about adding a guideline >>>>>>>> document >>>>>>>>> for writing tests, similar to how we have a coding style guide? It >>>>>> might >>>>>>>>> help ensure consistency across the Airflow codebase when it comes >> to >>>>>>>>> testing styles as well. >>>>>>>>> >>>>>>>>> 2025년 7월 5일 (토) 오후 8:52, Jarek Potiuk <ja...@potiuk.com>님이 작성: >>>>>>>>> >>>>>>>>>> But of course - i'd love to hear what others think - it's not a >>> "very >>>>>>>>>> strong" opinion. >>>>>>>>>> >>>>>>>>>> On Sat, Jul 5, 2025 at 1:48 PM Jarek Potiuk <ja...@potiuk.com> >>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> Cool. That's what I wanted to see. >>>>>>>>>>> >>>>>>>>>>> By the way - not that there's anything wrong - but was the >> answer >>>>>>>>> written >>>>>>>>>>> by AI initially ? The first paragraph looks suspiciously like >> Chat >>>>>>>> GPT >>>>>>>>>>> answer :D ? >>>>>>>>>>> >>>>>>>>>>> Comment from my side: I personally prefer the original style. >> It's >>>>>>>> more >>>>>>>>>>> concise and it is easier to read - you see as if the call was >>>>>>>> actually >>>>>>>>>>> written down. Also this is quite a bit too many assertions in >> the >>>>>>>>> second >>>>>>>>>>> case and it takes a lot of mental effort to understand what >>> actually >>>>>>>> is >>>>>>>>>>> being asserted. >>>>>>>>>>> >>>>>>>>>>> There is a "school" of writing unit tests that every test should >>>>>> have >>>>>>>>> ONE >>>>>>>>>>> assertion only. Always. I think it is a bit extreme, and I do >> not >>>>>>>>> follow >>>>>>>>>> it >>>>>>>>>>> myself but I think it is also a kind of good direction to have >> -> >>>>>> the >>>>>>>>>> fewer >>>>>>>>>>> assertions you have in your test, the better (I think). >>>>>>>>>>> >>>>>>>>>>> I think tests should be mostly optimized for easiness of reading >>> and >>>>>>>>>>> understanding what is being tested - and it's just not that easy >>> in >>>>>>>> the >>>>>>>>>>> second case. >>>>>>>>>>> >>>>>>>>>>> J. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Sat, Jul 5, 2025 at 1:39 PM Kyungjun Lee < >>>>>>>> kyungjunlee...@gmail.com> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> Sure! Since you asked for how the test *should* be written, I >>> took >>>>>>>> the >>>>>>>>>>>> opportunity to clean it up using a more pytest-native style >> while >>>>>>>> also >>>>>>>>>>>> fixing the mock order issue. >>>>>>>>>>>> >>>>>>>>>>>> Here’s the updated test: >>>>>>>>>>>> >>>>>>>>>>>> ```python >>>>>>>>>>>> >>>>>>>>>>>> @pytest.mark.parametrize( >>>>>>>>>>>> argnames="conn_id, account_id", >>>>>>>>>>>> argvalues=[(ACCOUNT_ID_CONN, None), (NO_ACCOUNT_ID_CONN, >>>>>>>>>> ACCOUNT_ID)], >>>>>>>>>>>> ids=["default_account", "explicit_account"], >>>>>>>>>>>> ) >>>>>>>>>>>> @patch.object(DbtCloudHook, "run") >>>>>>>>>>>> @patch.object(DbtCloudHook, "_paginate") >>>>>>>>>>>> def test_get_account(self, mock_paginate, mock_http_run, >> conn_id, >>>>>>>>>>>> account_id): >>>>>>>>>>>> hook = DbtCloudHook(conn_id) >>>>>>>>>>>> hook.get_account(account_id=account_id) >>>>>>>>>>>> >>>>>>>>>>>> assert hook.method == "GET" >>>>>>>>>>>> >>>>>>>>>>>> expected_account_id = account_id or DEFAULT_ACCOUNT_ID >>>>>>>>>>>> >>>>>>>>>>>> assert mock_http_run.call_count == 1 >>>>>>>>>>>> assert mock_http_run.call_args.args == () >>>>>>>>>>>> assert mock_http_run.call_args.kwargs == { >>>>>>>>>>>> "endpoint": f"api/v2/accounts/{expected_account_id}/", >>>>>>>>>>>> "data": None, >>>>>>>>>>>> "extra_options": None, >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> assert mock_paginate.call_count == 0 >>>>>>>>>>>> >>>>>>>>>>>> ``` >>>>>>>>>>>> Why I chose this style: >>>>>>>>>>>> >>>>>>>>>>>> - >>>>>>>>>>>> >>>>>>>>>>>> *Mock verification using assert*: Instead of >>>>>>>>>>>> mock.assert_called_once_with(...), I used call_count and >>>>>>>> call_args. >>>>>>>>>>>> This >>>>>>>>>>>> approach aligns better with pytest’s idioms and produces >>> cleaner, >>>>>>>>>> more >>>>>>>>>>>> readable error messages when assertions fail. >>>>>>>>>>>> - >>>>>>>>>>>> >>>>>>>>>>>> *Explicit verification*: Using call_args.args and >>>>>>>> call_args.kwargs >>>>>>>>>>>> makes >>>>>>>>>>>> the test behavior very explicit, which helps with debugging >> and >>>>>>>>>>>> understanding the exact calls made. >>>>>>>>>>>> - >>>>>>>>>>>> >>>>>>>>>>>> *Decorator order matching argument order*: As @patch >> decorators >>>>>>>>> apply >>>>>>>>>>>> from the bottom up, the argument order has been corrected to >>>>>>>> match >>>>>>>>> ( >>>>>>>>>>>> mock_paginate first, then mock_http_run). >>>>>>>>>>>> >>>>>>>>>>>> Let me know if you'd like to follow a slightly different >>> convention >>>>>>>> — >>>>>>>>>>>> happy >>>>>>>>>>>> to adjust! >>>>>>>>>>>> >>>>>>>>>>>> I was lucky to have the chance to explain this while fixing a >>>>>>>> related >>>>>>>>>> bug. >>>>>>>>>>>> You can refer to the changes in this PR: >>>>>>>>>>>> https://github.com/apache/airflow/pull/52905 >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> 2025년 7월 5일 (토) 오후 8:09, Jarek Potiuk <ja...@potiuk.com>님이 작성: >>>>>>>>>>>> >>>>>>>>>>>>> Just post how you think the test should be written :) >>>>>>>>>>>>> >>>>>>>>>>>>> On Sat, Jul 5, 2025 at 1:08 PM Jarek Potiuk <ja...@potiuk.com >>> >>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> I already mentioned it in slack - but how would you propose >> to >>>>>>>>>> rewrite >>>>>>>>>>>>> the >>>>>>>>>>>>>> "mixed" case to be more consistent ? >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Sat, Jul 5, 2025 at 12:47 PM Kyungjun Lee < >>>>>>>>>>>> kyungjunlee...@gmail.com> >>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi all, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> While reviewing and contributing to Airflow tests, I’ve >>> noticed >>>>>>>>> an >>>>>>>>>>>>>>> inconsistency in how assertions are written. Some tests use >>>>>>>>>>>>>>> `unittest`-style assertions like >>>>>>>> `mock.assert_called_once_with`, >>>>>>>>>>>> while >>>>>>>>>>>>>>> others use plain `assert` statements in the `pytest` style. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Here's an example of a test using the mixed style: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> ```python >>>>>>>>>>>>>>> @pytest.mark.parametrize( >>>>>>>>>>>>>>> argnames="conn_id, account_id", >>>>>>>>>>>>>>> argvalues=[(ACCOUNT_ID_CONN, None), (NO_ACCOUNT_ID_CONN, >>>>>>>>>>>>> ACCOUNT_ID)], >>>>>>>>>>>>>>> ids=["default_account", "explicit_account"], >>>>>>>>>>>>>>> ) >>>>>>>>>>>>>>> @patch.object(DbtCloudHook, "run") >>>>>>>>>>>>>>> @patch.object(DbtCloudHook, "_paginate") >>>>>>>>>>>>>>> def test_get_account(self, mock_http_run, mock_paginate, >>>>>>>> conn_id, >>>>>>>>>>>>>>> account_id): >>>>>>>>>>>>>>> hook = DbtCloudHook(conn_id) >>>>>>>>>>>>>>> hook.get_account(account_id=account_id) >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> assert hook.method == "GET" >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> _account_id = account_id or DEFAULT_ACCOUNT_ID >>>>>>>>>>>>>>> hook.run.assert_called_once_with( >>>>>>>>>>>>>>> endpoint=f"api/v2/accounts/{_account_id}/", data=None, >>>>>>>>>>>>>>> extra_options=None >>>>>>>>>>>>>>> ) >>>>>>>>>>>>>>> hook._paginate.assert_not_called() >>>>>>>>>>>>>>> ``` >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> In IDEs and type-checkers (like PyCharm or MyPy), this >>>>>>>> sometimes >>>>>>>>>>>> causes >>>>>>>>>>>>>>> weak warnings >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> ``` >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Cannot find reference 'assert_called_once_with' in >> 'function' >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> ``` >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> This could confuse newcomers or contributors unfamiliar with >>>>>>>>>> mocking >>>>>>>>>>>>>>> behavior or type limitations in dynamic typing. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> To improve clarity and accessibility for >>>>>>>> contributors—especially >>>>>>>>>>>> those >>>>>>>>>>>>> new >>>>>>>>>>>>>>> to the project—I’d like to propose *moving toward consistent >>>>>>>> use >>>>>>>>> of >>>>>>>>>>>>> plain >>>>>>>>>>>>>>> assert statements* for test validations wherever possible. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> *Proposed Benefits*: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> - >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Easier onboarding for first-time contributors >>>>>>>>>>>>>>> - >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Better IDE support and fewer confusing warnings >>>>>>>>>>>>>>> - >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> More consistent and readable test style across the project >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I'd love to hear your thoughts on whether this direction >> makes >>>>>>>>>> sense >>>>>>>>>>>> for >>>>>>>>>>>>>>> the project. If agreed, I’d also be happy to help align >>>>>>>> existing >>>>>>>>>>>> tests >>>>>>>>>>>>>>> gradually. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thanks! >>>>>>>>>>>>>>> Kyungjun Lee >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>> >>>>>> >>>>>> --------------------------------------------------------------------- >>>>>> 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