You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/01/13 06:44:36 UTC
[GitHub] [airflow] potiuk opened a new pull request #20843: Fix flaky templatized call
potiuk opened a new pull request #20843:
URL: https://github.com/apache/airflow/pull/20843
<!--
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 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 change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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 a change in pull request #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#discussion_r783765812
##########
File path: tests/sensors/test_python.py
##########
@@ -85,7 +85,7 @@ def test_python_callable_arguments_are_templatized(self):
ds_templated = DEFAULT_DATE.date().isoformat()
# 2 calls: first: at start, second: before timeout
- assert 2 == len(recorded_calls)
+ assert 1 <= len(recorded_calls)
Review comment:
I also removed it from the test below (same story but it had a comment about it and was already using >= 1).
--
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] uranusjr commented on a change in pull request #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#discussion_r783674157
##########
File path: tests/sensors/test_python.py
##########
@@ -85,7 +85,7 @@ def test_python_callable_arguments_are_templatized(self):
ds_templated = DEFAULT_DATE.date().isoformat()
# 2 calls: first: at start, second: before timeout
- assert 2 == len(recorded_calls)
+ assert 1 <= len(recorded_calls)
Review comment:
Why do we have the second call at all? Can we not have it?
--
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 #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#issuecomment-1011843612
Fixes flaky test (example in #20795) https://github.com/apache/airflow/runs/4793994282?check_suite_focus=true
--
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 a change in pull request #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#discussion_r783698559
##########
File path: tests/sensors/test_python.py
##########
@@ -85,7 +85,7 @@ def test_python_callable_arguments_are_templatized(self):
ds_templated = DEFAULT_DATE.date().isoformat()
# 2 calls: first: at start, second: before timeout
- assert 2 == len(recorded_calls)
+ assert 1 <= len(recorded_calls)
Review comment:
It' sensor and number of calls depends on how fast the whole system is at all. We know tht the first call happens for sure, and most of the time the second too. The assert is actually correct IMHO ("expect at least 1 call"). We only check the first call anyway (and this is the gist of it as we really want to see if JINJA template works in the sensor call).
If you think on how to fix it better I am al ears :)
--
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] uranusjr commented on a change in pull request #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#discussion_r783762319
##########
File path: tests/sensors/test_python.py
##########
@@ -85,7 +85,7 @@ def test_python_callable_arguments_are_templatized(self):
ds_templated = DEFAULT_DATE.date().isoformat()
# 2 calls: first: at start, second: before timeout
- assert 2 == len(recorded_calls)
+ assert 1 <= len(recorded_calls)
Review comment:
Let’s remove this assert entirely then. The `1 <=` part is validated below anyway (if there isn’t a call, `recorded_calls[0]` would automatically fail).
--
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 a change in pull request #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#discussion_r783698559
##########
File path: tests/sensors/test_python.py
##########
@@ -85,7 +85,7 @@ def test_python_callable_arguments_are_templatized(self):
ds_templated = DEFAULT_DATE.date().isoformat()
# 2 calls: first: at start, second: before timeout
- assert 2 == len(recorded_calls)
+ assert 1 <= len(recorded_calls)
Review comment:
It' sensor and number of calls depends on how fast the whole system is at all (sometimes we will not get the second one if the system is very busy with other parallell tests.
We know tht the first call happens for sure (it is immediate), and most of the time the second too. The assert is actually correct IMHO ("expect at least 1 call"). We only check the first call anyway (and this is the gist of it as we really want to see if JINJA template works in the sensor call).
If you think on how to fix it better I am all ears :)
--
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 a change in pull request #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#discussion_r783700863
##########
File path: tests/sensors/test_python.py
##########
@@ -85,7 +85,7 @@ def test_python_callable_arguments_are_templatized(self):
ds_templated = DEFAULT_DATE.date().isoformat()
# 2 calls: first: at start, second: before timeout
- assert 2 == len(recorded_calls)
+ assert 1 <= len(recorded_calls)
Review comment:
The thing here is that his is the "timing" tests and we test it with "run" method and expect timeout.
In perfect world, we could likely use freezegun and simulate the exact time passing, but IMHO it is totally not worth it if we can 100% fix it with this one character change. Trading test simplicity with perfectness.
--
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 #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #20843:
URL: https://github.com/apache/airflow/pull/20843
--
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 a change in pull request #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#discussion_r783698559
##########
File path: tests/sensors/test_python.py
##########
@@ -85,7 +85,7 @@ def test_python_callable_arguments_are_templatized(self):
ds_templated = DEFAULT_DATE.date().isoformat()
# 2 calls: first: at start, second: before timeout
- assert 2 == len(recorded_calls)
+ assert 1 <= len(recorded_calls)
Review comment:
It' sensor and number of calls depends on how fast the whole system is at all (sometimes we will not get the second one if the system is very busy with other parallell tests.
We know tht the first call happens for sure (it is immediate), and most of the time the second too. The assert is actually correct IMHO ("expect at least 1 call"). We only check the first call anyway (and this is the gist of it as we really want to see if JINJA template works in the sensor call).
If you think on how to fix it better I am al ears :)
--
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] github-actions[bot] commented on pull request #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#issuecomment-1011947901
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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 a change in pull request #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#discussion_r783763766
##########
File path: tests/sensors/test_python.py
##########
@@ -85,7 +85,7 @@ def test_python_callable_arguments_are_templatized(self):
ds_templated = DEFAULT_DATE.date().isoformat()
# 2 calls: first: at start, second: before timeout
- assert 2 == len(recorded_calls)
+ assert 1 <= len(recorded_calls)
Review comment:
Works for me :)
--
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] uranusjr commented on a change in pull request #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#discussion_r783673639
##########
File path: tests/sensors/test_python.py
##########
@@ -85,7 +85,7 @@ def test_python_callable_arguments_are_templatized(self):
ds_templated = DEFAULT_DATE.date().isoformat()
# 2 calls: first: at start, second: before timeout
- assert 2 == len(recorded_calls)
+ assert 1 <= len(recorded_calls)
Review comment:
This seems to defeat the purpose of this assertion. Why is the second call not recorded?
--
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 a change in pull request #20843: Fix flaky templatized call
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20843:
URL: https://github.com/apache/airflow/pull/20843#discussion_r783700863
##########
File path: tests/sensors/test_python.py
##########
@@ -85,7 +85,7 @@ def test_python_callable_arguments_are_templatized(self):
ds_templated = DEFAULT_DATE.date().isoformat()
# 2 calls: first: at start, second: before timeout
- assert 2 == len(recorded_calls)
+ assert 1 <= len(recorded_calls)
Review comment:
The thing here is that his is the "timing" tests and we test it with "run" method and expect timeout.
In perfect world, we could likely use freezegun and simulate the exact time passing, but IMHO it is totally not worth it if we can 100% fix it with this three characters change. Trading test simplicity with perfectness.
--
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