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