You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2023/02/12 10:03:20 UTC
[GitHub] [airflow] potiuk opened a new pull request, #29489: Fix failing multiple-output-inference tests for Python 3.8+
potiuk opened a new pull request, #29489:
URL: https://github.com/apache/airflow/pull/29489
The #29445 introduced a test for generated warnings that tested line number of the generated warning. However the number of line had changed in Python 3.8 because the stack trace of the error is a bit different.
The #29445 tests passed when merging as they were only run for Python 3.7, however our Canary builds caught it and failed in main.
Adding conditionals to handle it for other Python versions as well.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29489: Fix failing multiple-output-inference tests for Python 3.8+
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29489:
URL: https://github.com/apache/airflow/pull/29489#issuecomment-1437499603
> Just a thoughts is PEP-657 which introduced in [Python 3.11](https://docs.python.org/3/whatsnew/3.11.html#pep-657-fine-grained-error-locations-in-tracebacks) would potentially broke this test again.
Then we will fix it there :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29489: Fix failing multiple-output-inference tests for Python 3.8+
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29489:
URL: https://github.com/apache/airflow/pull/29489#issuecomment-1427050501
> This comment not for this test rather than situation with tests in general.
>
> I thought that is also a good idea to add docstring to tests cases, when we introduce new test case. When I tried to migrate some old tests from unittests to pytest I found that I have no idea what some test about
Well I think if we have a need to add docstring to a more than a handful of very specific cases that need a lot of context, then I believe that this is rather an indication that either:
a) test code is too complex and should written in better "Descriptive and meaningful" way (see https://enterprisecraftsmanship.com/posts/dry-damp-unit-tests/) discussion
or
b) test name or corresponding parameterized entries are not descriptive enough
IMHO name of the method of the test case (even if its 100 characters) should be descriptive enough to describe what the test is about. This is, I think, far better than docstring. If anything then I'd say in-line comments describing more context of particular lines of the test code are much, much better than test docstrings when the code is not enough to figure out context.
As I see it, the docstrings are really useful when you have an API-kind-of-class that you want people use without really having to loook at the code. Docstrings have huge potential of becoming out-dated and invalid, where the code is always doing what it is doing and what it tells. Usually code is far better of a documentation than the documentation itself (or it should be - and we should strive for it).
Test code should be distincly different than "production code" as well - less DRY, more descriptive, and it is main property is that it is supposed to be read by whoever who wants to do with it. So it's better if the code speaks to whoever looks at it rather than docstring.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29489: Fix failing multiple-output-inference tests for Python 3.8+
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29489:
URL: https://github.com/apache/airflow/pull/29489#issuecomment-1432911830
Just a thoughts is PEP-657 which introduced in [Python 3.11](https://docs.python.org/3/whatsnew/3.11.html#pep-657-fine-grained-error-locations-in-tracebacks) would potentially broke this test again
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #29489: Fix failing multiple-output-inference tests for Python 3.8+
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29489:
URL: https://github.com/apache/airflow/pull/29489
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29489: Fix failing multiple-output-inference tests for Python 3.8+
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29489:
URL: https://github.com/apache/airflow/pull/29489#issuecomment-1437535260
> Then we will fix it there :)
That why I mention this PR, might be it would save some time when it time come to Python 3.11
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org