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

Remove upper bound limitation for pytest #29086

Merged
merged 4 commits into from
Jan 23, 2023
Merged

Remove upper bound limitation for pytest #29086

merged 4 commits into from
Jan 23, 2023

Conversation

auvipy
Copy link
Contributor

@auvipy auvipy commented Jan 21, 2023

An attempt to update pytest version limit

setup.py Outdated
@@ -387,7 +387,7 @@ def write_version(filename: str = str(AIRFLOW_SOURCES_ROOT / "airflow" / "git_ve
# It contains a number of potential breaking changes but none of them looks breaking our use
# https://docs.pytest.org/en/latest/changelog.html#pytest-7-0-0-2022-02-03
# TODO: upgrade it and remove the limit
"pytest~=6.0",
"pytest~=7.2.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if main idea remove upper bound limitations and better use pytest>=6.0 or pytest>=7.0 (if we intend to use features which added only in 7.0 branch)

Maybe it is not a case of pytest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense, done

setup.py Outdated Show resolved Hide resolved
@auvipy auvipy marked this pull request as ready for review January 21, 2023 16:23
@Taragolis
Copy link
Contributor

I have a look at the warning logs, It is very hard to find where actually it raised. But finally I found only one type of pytest-specific warning.

When we won't expected any warnings in test case we use

with pytest.warns(None):
    ...

But we should: https://docs.pytest.org/en/latest/how-to/capture-warnings.html#additional-use-cases-of-warnings-in-tests

with warnings.catch_warnings():
    warnings.simplefilter("error")
    ...

We should do this as follow up however I have a suspicion that recommended solution wouldn't work because pytest-capture-warnings suppress this errored-warning, see: #28590 (comment)

@Taragolis
Copy link
Contributor

@auvipy could you also remove this upper bound limitation

airflow/setup.py

Lines 395 to 397 in 245c4e3

# We should attempt to remove the limit when we upgrade Pytest
# TODO: remove the limit when we upgrade pytest
"pytest-rerunfailures~=9.1",

I think we use it because version 10 required pytest 7+

@Taragolis Taragolis changed the title try "pytest~=7.2.1" Remove upper bound limitation for pytest Jan 21, 2023
@auvipy
Copy link
Contributor Author

auvipy commented Jan 22, 2023

yes of course, thanks for pointing

@potiuk
Copy link
Member

potiuk commented Jan 22, 2023

NIce. Seems like everything works fine. Test seems to be executing and succeeding, I see not even yellow flags. Pytest is upgraded in the image:

Screenshot 2023-01-22 at 14 55 56

All looks good.

@potiuk
Copy link
Member

potiuk commented Jan 22, 2023

(pending removal of rerun plugin upper-bound limit of course)

@auvipy
Copy link
Contributor Author

auvipy commented Jan 22, 2023

ok will do that

@Taragolis
Copy link
Contributor

ok will do that

Also need remove redundant comments that we need to migrate.

@auvipy
Copy link
Contributor Author

auvipy commented Jan 22, 2023

ok will clear the comments as well

@auvipy
Copy link
Contributor Author

auvipy commented Jan 23, 2023

done with review comments

@Taragolis
Copy link
Contributor

Non relevant error in the CI to this PR. Looks like internal_api test flaky

            if not ignore_running:
                raise AssertionError(
>                   "Background processes are running that prevent the test from passing successfully."
                )
E               AssertionError: Background processes are running that prevent the test from passing successfully.

tests/cli/commands/test_internal_api_command.py:112: AssertionError

Copy link
Contributor

@Taragolis Taragolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@auvipy
Copy link
Contributor Author

auvipy commented Jan 23, 2023

I think we can handle that in another PR?

@Taragolis
Copy link
Contributor

Taragolis commented Jan 23, 2023

I think we can handle that in another PR?

It definitely for another PR and related only to AIP-44, this flaky error also appeared in pytest 6 yesterday.

@potiuk potiuk merged commit c0f6bfa into apache:main Jan 23, 2023
@auvipy auvipy deleted the patch-1 branch January 23, 2023 16:24
@auvipy
Copy link
Contributor Author

auvipy commented Jan 23, 2023

thank you both

@pierrejeambrun pierrejeambrun added this to the Airflow 2.5.2 milestone Feb 27, 2023
@pierrejeambrun pierrejeambrun added the type:misc/internal Changelog: Misc changes that should appear in change log label Feb 27, 2023
pierrejeambrun pushed a commit that referenced this pull request Mar 7, 2023
pierrejeambrun pushed a commit that referenced this pull request Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants