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