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

Unit test for celery change_state wrapper #40035

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

o-nikolas
Copy link
Contributor

@o-nikolas o-nikolas commented Jun 4, 2024

If an older version of main is being used BaseExecutor.change_state may have fewer parameters, catch the TypeError that is raised in this situation instead of the previously caught AttributeError.

Add a unit test to cover this scenario

fixes #40011
fixes #39980
related #40012 - This PR solves the same issue, but didn't include tests. I spent some time today noodling around with a test case and it turned out to be stubborn. So I'm opening this PR. It can either be merged or used as inspiration for #40012


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@o-nikolas
Copy link
Contributor Author

CC @eladkal @leakec

@leakec
Copy link
Contributor

leakec commented Jun 4, 2024

@o-nikolas I'm very unfamiliar with the airflow test case suite (I'm new to airflow in general), so I doubt I'll get much further in terms of writing a test without a significant time sink. Similarly, I wasn't sure if there were any cases where AttributeError could still be raised, it didn't look like that to me, but I left both in there just in case, i.e., #40012 catches both exception types rather than replacing AttributeError with TypeError.

If AttributeError will not be thrown and TypeError is the one that will always be raised here (looks like that to me), then this PR is a better fix than #40012, and it has a test case, which #40012 does not. If this test case captures everything (to me it looks like it does), then I think we should just close #40012 without merging and merge this one instead.

@o-nikolas
Copy link
Contributor Author

@o-nikolas I'm very unfamiliar with the airflow test case suite (I'm new to airflow in general), so I doubt I'll get much further in terms of writing a test without a significant time sink. Similarly, I wasn't sure if there were any cases where AttributeError could still be raised, it didn't look like that to me, but I left both in there just in case, i.e., #40012 catches both exception types rather than replacing AttributeError with TypeError.

If AttributeError will not be thrown and TypeError is the one that will always be raised here (looks like that to me), then this PR is a better fix than #40012, and it has a test case, which #40012 does not. If this test case captures everything (to me it looks like it does), then I think we should just close #40012 without merging and merge this one instead.

It looks like @potiuk has merged it anyway so problem solved 😆

I'll rebase this PR, because it still adds a test case, which we can merge or not, let's see what the community thinks.

A unit test which triggers the scenario of a current Celery executor
with the new signature of change_state run against an older version of
Airflow with the old signature of change_state on the BaseExecutor
class.

related apache#40011
related apache#39980
related apache#40012
@o-nikolas o-nikolas force-pushed the onikolas/celery_change_state_fix branch from 3646ac0 to 9075846 Compare June 4, 2024 20:38
@leakec
Copy link
Contributor

leakec commented Jun 5, 2024

@o-nikolas I think adding the test case is definitely worth it. Also, would be great to get feedback on whether that exception should be

except TypeError:

or

except (AttributeError, TypeError):

If it's the former, we should merge what you have in.

@o-nikolas o-nikolas changed the title Catch TypeError instead of AttributeError for change_state wrapper Unit test for celery change_state wrapper Jun 5, 2024
@eladkal eladkal self-requested a review June 5, 2024 17:10
@eladkal eladkal merged commit 4dea367 into apache:main Jun 5, 2024
50 checks passed
@potiuk potiuk added this to the Airflow 2.9.2 milestone Jun 6, 2024
fdemiane pushed a commit to fdemiane/airflow that referenced this pull request Jun 6, 2024
A unit test which triggers the scenario of a current Celery executor
with the new signature of change_state run against an older version of
Airflow with the old signature of change_state on the BaseExecutor
class.

related apache#40011
related apache#39980
related apache#40012
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
A unit test which triggers the scenario of a current Celery executor
with the new signature of change_state run against an older version of
Airflow with the old signature of change_state on the BaseExecutor
class.

related apache#40011
related apache#39980
related apache#40012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Numerous error reports in airflow logs due to function argument mismatch
6 participants