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

Reply via email to