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