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