Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade pytest to v8 #37151

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Upgrade pytest to v8 #37151

wants to merge 10 commits into from

Conversation

hussein-awala
Copy link
Member

No description provided.

@boring-cyborg boring-cyborg bot added area:dev-tools area:production-image Production image improvements and fixes provider:cncf-kubernetes Kubernetes provider related issues labels Feb 2, 2024
@hussein-awala
Copy link
Member Author

depends on: pytest-dev/pytest-asyncio#765

@Taragolis Taragolis linked an issue Feb 3, 2024 that may be closed by this pull request
2 tasks
@hussein-awala
Copy link
Member Author

It will be fixed in the next release
pytest-dev/pytest-asyncio#776

@potiuk
Copy link
Member

potiuk commented Feb 11, 2024

Hey @hussein-awala -> the big problem is pytest-httpx. The latest version we can install in Python 3.8 is 0.23.1 (0.24 and above is Python >= 3.9. The 0.23.1 has Pytest < 8 so pip simply cannot find solution - there is no matching pytest-httpx that we could install in Python 3.8 image. There are likely other problems once we solve this one, but this is a basic prerequisite.

@potiuk
Copy link
Member

potiuk commented Feb 11, 2024

There is a exactly 1(!) test that uses httpx_mock - so I think we should remove pytest-httpx and implement the test differently

test_info_command.py:

    def test_show_info_anonymize_fileio(self, httpx_mock, setup_parser):

@potiuk
Copy link
Member

potiuk commented Feb 11, 2024

PR removing pytest-httpx here: #37334 - once we merge it , we can try again.

@potiuk
Copy link
Member

potiuk commented Feb 11, 2024

For the future @hussein-awala -

One easy way to test if a new dependency is not conflicting is quite simple:

  1. Update pyproject toml
  2. Enter Breeze
  3. Run pip install -e ".[all]" YOUR_DEPENDENCY==VERSION - where YOUR_DEPENDENCY==VERSION is the version you think should be installed by breeze. In this case:
pip install -e ".[all]" pytest==8.0.0

This should find if there is any conflict, much quicker (because you specifically choose dependency version and pip will not try to backtrack and find other combinations, it will simply tell you what's wrong.

In this case the message was:

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
pytest-httpx 0.21.3 requires pytest<8.0,>=6.0, but you have pytest 8.0.0 which is incompatible.

This is how I found out pytest-httpx is problematic.

@hussein-awala
Copy link
Member Author

Thanks Jarek for this detailed explanation and for the tip to test the future changes.

@hussein-awala
Copy link
Member Author

I'm still trying to understand why tests/utils/log/test_colored_log.py::test_format_time_uses_tz_aware fails in Breeze the first time I run it, but it works locally and in Breeze if I run it more than once in the same terminal 🤔

Changing the logging level and moving the logger to a fixture did not resolve the issue. I need to thoroughly check the pytest changelog to understand what's going on.

@potiuk
Copy link
Member

potiuk commented Feb 13, 2024

@hussein-awala : change patching: airflow.utils.log.timezone_aware.TimezoneAware.formatTime -> airflow.utils.log.colored_log.TimezoneAware.formatTime ?

@hussein-awala
Copy link
Member Author

yes, I tried that too, but it didn't work.

@potiuk
Copy link
Member

potiuk commented Feb 13, 2024

This one is strange :)

@Taragolis
Copy link
Contributor

Taragolis commented Feb 13, 2024

Try to add format which include asctime attribute into format into the formatter

h.setFormatter(CustomTTYColoredFormatter())

Something like that

    h.setFormatter(CustomTTYColoredFormatter(fmt="%(asctime)s: %(log_color)s%(message)s%(reset)s"))

@hussein-awala
Copy link
Member Author

Try to add format which include asctime attribute into format into the formatter

h.setFormatter(CustomTTYColoredFormatter())

Something like that

    h.setFormatter(CustomTTYColoredFormatter(fmt="%(asctime)s: %(log_color)s%(message)s%(reset)s"))

It seems to work with this, which is more strange to me 🤔 Let's wait for the CI

@potiuk
Copy link
Member

potiuk commented Feb 13, 2024

It seems to work with this, which is more strange to me 🤔 Let's wait for the CI

Ah - interesting, that would mean that the default logging configuration is not initialized the first time you enter them (because airflow configuration is not writen to airflow.cfg yet.

@potiuk
Copy link
Member

potiuk commented Feb 13, 2024

Or smth equally racy

@Taragolis
Copy link
Contributor

One of the possible reason changes in collection order: pytest-dev/pytest#7777

Other changes in pytest 8: https://docs.pytest.org/en/stable/changelog.html

@hussein-awala hussein-awala changed the title [WIP] Upgrade pytest to v8 Upgrade pytest to v8 Feb 13, 2024
@hussein-awala hussein-awala marked this pull request as ready for review February 13, 2024 23:47
@potiuk
Copy link
Member

potiuk commented Feb 14, 2024

Nice and looks green :)

@potiuk
Copy link
Member

potiuk commented Feb 14, 2024

TOO QUICK :)

@Taragolis
Copy link
Contributor

Seems like, configs from providers do not loaded for this tests.

@Taragolis
Copy link
Contributor

Worthwhile to try it again with 8.1.1, it might fix some issues.

@Taragolis
Copy link
Contributor

Looks promising only couple of tests:

  1. pytest.warns(expected_warning=None) I miss that one when did Get rid of deprecated pytest.warns(None) usage #38027
  2. FAB tests

@potiuk
Copy link
Member

potiuk commented Mar 16, 2024

Getting closer :)

@Taragolis
Copy link
Contributor

Taragolis commented Mar 16, 2024

I could reproduce FAB tests failure in pytest 7 if I run this module in random order

root@d9cbab8b3875:/opt/airflow# pip install pytest-random-order

root@d9cbab8b3875:/opt/airflow# pytest tests/providers/fab/auth_manager/test_security.py --random-order
...
======================================================================= 3 failed, 56 passed, 20 warnings in 7.76s =======================================================================

root@d9cbab8b3875:/opt/airflow# pytest tests/providers/fab/auth_manager/test_security.py --random-order

======================================================================= 7 failed, 52 passed, 20 warnings in 7.91s =======================================================================

root@d9cbab8b3875:/opt/airflow# pytest tests/providers/fab/auth_manager/test_security.py
...
============================================================================ 59 passed, 20 warnings in 7.81s ============================================================================

100% there is some side effect which make tests work in this case, in pytest 8 the order slightly different so that is why the error happen into the CI

@hussein-awala
Copy link
Member Author

789c99a could fix it while keeping the test logic correct

@hussein-awala
Copy link
Member Author

I also fixed it by changing the fixtures scope from module to function, but it increased the test processing time. We need to add a teardown block somewhere to clean the db.

@Taragolis
Copy link
Contributor

Taragolis commented Mar 16, 2024

I also fixed it by changing the fixtures scope from module to function, but it increased the test processing time. We need to add a teardown block somewhere to clean the db.

I've tried to do the same things locally, but still have a side effects in random order

@pytest.fixture(autouse=True)
def clear_db_after_suite(app):
    clear_db_runs()
    clear_db_dags()
    delete_users(app)
    delete_roles(app)
    yield
    delete_users(app)
    delete_roles(app)
    clear_db_runs()
    clear_db_dags()

for example this two test in that order effect to each other

pytest "tests/providers/fab/auth_manager/test_security.py::TestHasAccessDagDecorator::test_dag_id_consistency[a-None-None-a-False]" tests/providers/fab/auth_manager/test_security.py::test_permissions_work_for_dags_with_dot_in_dagname

@Taragolis
Copy link
Contributor

But yeah if fix help we just need to create a task for a properly fix this test module

Copy link

github-actions bot commented May 2, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label May 2, 2024
@eladkal eladkal mentioned this pull request May 6, 2024
@github-actions github-actions bot closed this May 7, 2024
@hussein-awala hussein-awala reopened this May 7, 2024
@hussein-awala hussein-awala removed the stale Stale PRs per the .github/workflows/stale.yml policy file label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools area:production-image Production image improvements and fixes provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for pytest 8.0.0+
4 participants