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 <[email protected]>
> 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 <[email protected]>
> Sent: Tuesday, July 8, 2025 2:13 AM
> To: [email protected]
> 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 <[email protected]>님이 작성:
>
>> 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 <[email protected]> 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 <[email protected]>
>> 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 <[email protected]>님이 작성:
>>>>
>>>>> 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 <[email protected]>님이 작성:
>>>>>
>>>>>> 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 <[email protected]>
>>> 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 <[email protected]>님이 작성:
>>>>>>>
>>>>>>>>> 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 <
>>> [email protected]
>>>>>>>
>>>>>>>>> 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 <[email protected]>님이 작성:
>>>>>>>>>
>>>>>>>>>> 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 <[email protected]>
>>>>>> 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 <
>>>>>>>> [email protected]>
>>>>>>>>>>> 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 <[email protected]>님이 작성:
>>>>>>>>>>>>
>>>>>>>>>>>>> Just post how you think the test should be written :)
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Sat, Jul 5, 2025 at 1:08 PM Jarek Potiuk <[email protected]
>>>
>>>>>>>>> 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 <
>>>>>>>>>>>> [email protected]>
>>>>>>>>>>>>>> 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: [email protected]
>>>>>> For additional commands, e-mail: [email protected]
>>>>>>
>>>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [email protected]
>>> For additional commands, e-mail: [email protected]
>>>
>>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
