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