Thank you Ash and Amogh Desai for your insights and explanations. The information you shared has been incredibly helpful and is contributing a lot to my growth.
2025년 7월 8일 (화) 오후 2:54, Amogh Desai <amoghdesai....@gmail.com>님이 작성: > Agreed, I personally also find the current way to be easier to read and in > most > cases we want to assert if "something" was called, irrespective of the > order it was > called in. So the dedicated function based way works well for me. > > If I want to test a order, I'd rather call parts of code that I want to > test explicitly and assert > on them. > > > This happens because the mock object is not properly recognized as a mock > instance at type-checking time, which might confuse some contributors, > especially new ones relying on type hints or static analysis tools. > > Regarding this, I'd say you can either cast mocks to their types as one > way: > `mock_http_run: MagicMock = mock_http_run` -- give or take, or use > `autospec` to make the mock reflect the signature of the object? Check out: > https://docs.python.org/3/library/unittest.mock.html#autospeccing > > Thanks & Regards, > Amogh Desai > > > On Mon, Jul 7, 2025 at 6:13 PM Ash Berlin-Taylor <a...@apache.org> wrote: > > > def test_get_account(self, mock_paginate: Mock, mock_http_run: Mocj, > > conn_id, account_id): > > > > > > Might fix that? IDEs in general do not cope well with purest tests, and > > are missing context on what most of the variables are, be it > parameterised > > values or fixture values, so this isn’t a problem that is unique to > mocks. > > > > > On 7 Jul 2025, at 12:47, Kyungjun Lee <kyungjunlee...@gmail.com> > wrote: > > > > > > I'd like to follow up on our previous discussion about pytest-native vs > > > unittest-style assertions. > > > > > > While working on the following test case: > > > > > > ```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" > > > > > > _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() > > > > > > ``` > > > > > > My IDE shows a warning: > > > Cannot find reference 'assert_called_once_with' in 'function'. > > > > > > This happens because the mock object is not properly recognized as a > mock > > > instance at type-checking time, which might confuse some contributors, > > > especially new ones relying on type hints or static analysis tools. > > > > > > I realized that this aspect of mock handling might be missing from our > > > previous discussions. I wanted to bring it up as part of the broader > > > conversation about test styles—particularly how we balance IDE/tooling > > > support with readability and style consistency. > > > > > > Curious to hear your thoughts on this! > > > > > > @ash @potiuk > > > > > > 2025년 7월 6일 (일) 오후 8:09, Kyungjun Lee <kyungjunlee...@gmail.com>님이 작성: > > > > > >> Thank you @Potiuk for pointing out the intent behind the “one assert > per > > >> test” principle, and @ash for highlighting how using dedicated mock > > assert > > >> functions can make the code easier to read and understand. These were > > >> perspectives I hadn’t fully considered, and I really appreciate you > > sharing > > >> them. > > >> > > >> Thanks to your input, I was able to read more materials and broaden my > > >> thinking on the topic. That said, my original focus was more on the > idea > > >> that sticking to plain assert statements lowers the entry barrier for > > new > > >> contributors—because they don’t have to learn multiple assertion > styles. > > >> > > >> Still, as @Potiuk mentioned, I’ll put more thought into making the > > testing > > >> guidelines clearer and more concrete—especially since that helps > > AI-based > > >> tools as well 😄 > > >> > > >> For reference, here’s one of the articles I read: > > >> https://medium.com/@kentbeck_7670/test-desiderata-94150638a4b3 > > >> > > >> Thank you to everyone who took part in this discussion. > > >> > > >> 2025년 7월 6일 (일) 오후 3:42, Ash Berlin-Taylor <a...@apache.org>님이 작성: > > >> > > >>> I personally find the dedicated functions way easier to read the > intent > > >>> behind, it’s one function/statement vs 3. More so when you don’t care > > about > > >>> the order of calls, just that something was called (where to do this > > >>> manually we’d need to reimplement the helper function) > > >>> > > >>> Additionally pytest already rewrites those to have nicer error > > messages, > > >>> but the dedicated mock assert finding are much easier to read the > code > > and > > >>> understand the intent to me, so I’i am for staying with the dedicated > > >>> assert functions > > >>> > > >>> -ash > > >>> > > >>>> On 5 Jul 2025, at 13:30, Kyungjun Lee <kyungjunlee...@gmail.com> > > wrote: > > >>>> > > >>>> I’ve learned a lot of things I didn’t know before. > > >>>> Thank you so much for all your help — I really appreciate it. > > >>>> I’ll get started on this right away! > > >>>> > > >>>> 2025년 7월 5일 (토) 오후 9:18, Jarek Potiuk <ja...@potiuk.com>님이 작성: > > >>>> > > >>>>>> 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. > > >>>>> > > >>>>> That whole paragraph :) . > > >>>>> > > >>>>>>> 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. > > >>>>> > > >>>>> "Sure! Since you asked ..." sounds like an AI bot. > > >>>>> > > >>>>>> 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? > > >>>>> > > >>>>> Absolutely - we already have some "seed' of it "Writing tests" > > >>> chapter in > > >>>>> contributing guideline, but we could definitely make it more > > detailed. > > >>>>> > > >>>>> > > >>>>> > > >>> > > > https://github.com/apache/airflow/blob/main/contributing-docs/testing/unit_tests.rst#writing-unit-tests > > >>>>> > > >>>>> And - speaking of AI - this is becoming more and more important to > > >>> describe > > >>>>> any common rules we have and context - so that using Agentic AI > > yields > > >>>>> better results. Kaxil already added > > >>>>> https://github.com/apache/airflow/blob/main/AGENTS.md which > > describes > > >>>>> context for coding agents lile Codex - and we could improve it and > > link > > >>>>> more docs from our repo if they get more of our agreed > "conventions" > > - > > >>> then > > >>>>> Agents would get it as context and their generated code would be > > >>> consistent > > >>>>> with what we describe there. > > >>>>> > > >>>>> In a way - I think having a good documentation on processes, tools > > and > > >>>>> conventions was always something I've been after, but with the > > Agentic > > >>>>> workflows it might significantly boost the quality of generated > code > > >>> if we > > >>>>> have more of those conventions and guidelines described. > > >>>>> > > >>>>> So .... ABSOLUTELY ... the more we describe in there, the better. > And > > >>> we > > >>>>> have no more excuse that "anyhow no-one reads it" - because the > > coding > > >>>>> agents WILL be reading it and acting accordingly. So I think this > is > > a > > >>> very > > >>>>> good investment to make. > > >>>>> > > >>>>> J. > > >>>>> > > >>>>> > > >>>>> > > >>>>> > > >>>>> > > >>>>>> On Sat, Jul 5, 2025 at 2:07 PM Kyungjun Lee < > > kyungjunlee...@gmail.com > > >>>> > > >>>>>> wrote: > > >>>>>> > > >>>>>> 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 > > >>>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>> > > >>>>>>>> > > >>>>>>> > > >>>>>> > > >>>>> > > >>> > > >>> > > >>> --------------------------------------------------------------------- > > >>> To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > >>> For additional commands, e-mail: dev-h...@airflow.apache.org > > >>> > > >>> > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > For additional commands, e-mail: dev-h...@airflow.apache.org > > > > >
