potiuk commented on PR #30315:
URL: https://github.com/apache/airflow/pull/30315#issuecomment-1486828899
> Not only module name could be the reason that tests are not collected. The
also other naming conventions also could be a reason, and we also have it in
the past when test class doesn't start with Test*
Yep. But missing test_ in module name is far more difficult to catch by
contributor.
If you don't have Test* name (unlike module name) you won't be able to run
this test in your IDE or command line if you don't follow the Test* convention.
That's the big difference because not following `test_` module name might (and
it already happened with smtp and datbricks) be not noticed by the contributors
when adding their tests. Most people will run tests either via IDE or by runing
`pytest tests/your_test_path/your_test_module.py` to test if they are working.
This is also nicely supported by auto-completion in command line (and this
auto-completion is based on shell autocompletion of file names so it does not
know how to filter out files that do not follow `test_` prefixs..
Auto-completing test class/test name with the cumbersome "::" syntax is not
working. And even if somebody used it - it still does not run.
I just made an experiment. I brought back "test_*.py" in my PR and I run run
this:
```
pytest tests/providers/smtp/hooks/smtp.py
/home/jarek/.pyenv/versions/3.7/envs/airflow-3.7/lib/python3.7/site-packages/pytest_asyncio/plugin.py:177:
DeprecationWarning: You're using an outdated version of pytest. Newer releases
of pytest-asyncio will not be compatible with this pytest version. Please
update pytest to version 7 or later.
DeprecationWarning,
========================================================= test session
starts =========================================================
platform linux -- Python 3.7.16, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 --
/home/jarek/.pyenv/versions/3.7/envs/airflow-3.7/bin/python3
cachedir: .pytest_cache
rootdir: /home/jarek/code/airflow, configfile: pyproject.toml
plugins: rerunfailures-9.1.1, requests-mock-1.10.0, timeouts-1.2.1,
xdist-3.1.0, instafail-0.4.2, cov-4.0.0, anyio-3.6.2, httpx-0.21.2,
asyncio-0.20.3, time-machine-2.9.0
asyncio: mode=strict
setup timeout: 0.0s, execution timeout: 0.0s, teardown timeout: 0.0s
collected 19 items
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_connect_and_disconnect
PASSED [ 5%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_connect_and_disconnect_via_nonssl
PASSED [ 10%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_get_email_address_single_email
PASSED [ 15%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_get_email_address_comma_sep_string
PASSED [ 21%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_get_email_address_colon_sep_string
PASSED [ 26%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_get_email_address_list
PASSED [ 31%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_get_email_address_tuple
PASSED [ 36%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_get_email_address_invalid_type
PASSED [ 42%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_get_email_address_invalid_type_in_iterable
PASSED [ 47%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_build_mime_message
PASSED [ 52%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_send_smtp PASSED
[ 57%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_hook_conn PASSED
[ 63%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_send_mime_ssl PASSED
[ 68%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_send_mime_nossl
PASSED [ 73%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_send_mime_noauth
PASSED [ 78%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_send_mime_dryrun
PASSED [ 84%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_send_mime_ssl_complete_failure
PASSED [ 89%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_send_mime_partial_failure
PASSED [ 94%]
tests/providers/smtp/hooks/smtp.py::TestSmtpHook::test_send_mime_custom_timeout_retrylimit
PASSED [100%]
========================================================= 19 passed in 0.42s
==========================================================
```
Also the tests are nicely showing as Runnable tests in the IDE (regardless
of pyproject.toml setting):
Now, I renamed TestSmtpHook to SmtpHook and I got this:
```
pytest tests/providers/smtp/hooks/smtp.py
/home/jarek/.pyenv/versions/3.7/envs/airflow-3.7/lib/python3.7/site-packages/pytest_asyncio/plugin.py:177:
DeprecationWarning: You're using an outdated version of pytest. Newer releases
of pytest-asyncio will not be compatible with this pytest version. Please
update pytest to version 7 or later.
DeprecationWarning,
========================================================= test session
starts =========================================================
platform linux -- Python 3.7.16, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 --
/home/jarek/.pyenv/versions/3.7/envs/airflow-3.7/bin/python3
cachedir: .pytest_cache
rootdir: /home/jarek/code/airflow, configfile: pyproject.toml
plugins: rerunfailures-9.1.1, requests-mock-1.10.0, timeouts-1.2.1,
xdist-3.1.0, instafail-0.4.2, cov-4.0.0, anyio-3.6.2, httpx-0.21.2,
asyncio-0.20.3, time-machine-2.9.0
asyncio: mode=strict
setup timeout: 0.0s, execution timeout: 0.0s, teardown timeout: 0.0s
collected 0 items
======================================================== no tests ran in
0.02s ========================================================
```
I even used the colon syntax when I renamed the class:
```
ytest tests/providers/smtp/hooks/smtp.py::SmtpHook
/home/jarek/.pyenv/versions/3.7/envs/airflow-3.7/lib/python3.7/site-packages/pytest_asyncio/plugin.py:177:
DeprecationWarning: You're using an outdated version of pytest. Newer releases
of pytest-asyncio will not be compatible with this pytest version. Please
update pytest to version 7 or later.
DeprecationWarning,
====================================================================================================================================================================================
test session starts
=====================================================================================================================================================================================
platform linux -- Python 3.7.16, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 --
/home/jarek/.pyenv/versions/3.7/envs/airflow-3.7/bin/python3
cachedir: .pytest_cache
rootdir: /home/jarek/code/airflow, configfile: pyproject.toml
plugins: rerunfailures-9.1.1, requests-mock-1.10.0, timeouts-1.2.1,
xdist-3.1.0, instafail-0.4.2, cov-4.0.0, anyio-3.6.2, httpx-0.21.2,
asyncio-0.20.3, time-machine-2.9.0
asyncio: mode=strict
setup timeout: 0.0s, execution timeout: 0.0s, teardown timeout: 0.0s
collected 0 items
===================================================================================================================================================================================
no tests ran in 0.01s
====================================================================================================================================================================================
```
And neither test class nor test methods are runnable via IDE any more:
This is PRECISELY what happened when smtp tests were added (and maybe
@hussein-awala can confirm it).
* smtp.py module is nicely runnable via IDE or command line and this is what
made the author thought it is perfectly fine
* yet it is not discovered during CI run
* and there is absolutley no warning aboout it
So this is what I want to avoid - I do not want to enforce standards (that
might be follow up) - I want to make sure tha human mistakes are not causing
regressions.
> May be just better follow naming conventions (this sample only cover
regular airflow parts but not breeze, k8s and charts test) and check it again
Sure. I absolutely don't argue with that. Even more - i will absolutely
support it. Feel absolutely free to follow that. It will require quite a number
of catch-up with the modules that do not follow it. a lot of manual fixes and
renames. But it's an approach we might take in the future. I will be certainly
supportive and happilly take part in it :). It will likely take weeks ro months
to complete though and likely quite some communication and multiple people to
contribute.
And I think this is a nice follow-up to he the change here and this change
does not invalidate this one. It's done, green, functional. Standardized using
`pyproject.toml` which is the new standard and fixes some of the problems along
the waye. It solves the problem at-hand that it will avoid some of the problems
that happened even recently (the smtp thing happened literally 2 weeks ago.
I believe merging (or not) this change does not change anything with the
idea of adding conventions? We can still add it even if we merge this one I
guess? Or do you think this change is problematic if we would like to follow
with implementing the convention you mentioned?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
