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
[AIRFLOW-2557] Remove paged test for s3 #3455
Conversation
You could reduce the number from 5000 to 1001, as a single call will return at most 1000 results. |
Codecov Report
@@ Coverage Diff @@
## master #3455 +/- ##
==========================================
- Coverage 76.89% 76.89% -0.01%
==========================================
Files 203 203
Lines 15118 15120 +2
==========================================
+ Hits 11625 11626 +1
- Misses 3493 3494 +1
Continue to review full report at Codecov.
|
@NielsZeilemaker I know, but I looked at the code and I don’t think that makes sense. We are not testing anything inside airflow by it and the tests together would still take around 20seconds. |
Well, it verifies this logic: https://github.com/apache/incubator-airflow/blob/master/airflow/hooks/S3_hook.py#L98-L102 |
No it doesn’t, that part will always be tested as you can see in the test coverage as well. There will be always one page if you have results. |
It looks like the branch is still being hit: https://codecov.io/gh/apache/incubator-airflow/pull/3455/src/airflow/hooks/S3_hook.py#L98...102 🤔 |
It will, as you always have a single page, but testing to see if multiple pages work would have my preference, as I actually encountered this bug while using the hook. Maybe there is a quicker method to add the files? |
Can you lower the page size somehow? |
Paged tests for s3 are taking over 120 seconds. There is functionality to set the page size. This reduces the time spent on tests.
714f050
to
a117d1a
Compare
@NielsZeilemaker @Fokko updated. Page size is a supported setting Note: @Fokko lets not accept PRs anymore that go over 2s in tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bolkedebruin, looking good. Codecov tells me that the branch is still being hit.
I agree that normal tests should run within 2seconds, maybe drop a line on the dev mailinglist?
Paged tests for s3 are taking over 120 seconds. There is functionality to set the page size. This reduces the time spent on tests. Closes apache#3455 from bolkedebruin/AIRFLOW-2557
Paged tests for s3 are taking over 120 seconds. There is functionality to set the page size. This reduces the time spent on tests. Closes apache#3455 from bolkedebruin/AIRFLOW-2557
Make sure you have checked all steps below.
JIRA
Description
Paged tests for s3 are taking over 120 seconds. The
functionality testes is part of the standard boto suite
and part of its own tests.
Tests
tests removed.
Commits
Documentation
Code Quality
git diff upstream/master -u -- "*.py" | flake8 --diff
cc @Fokko @NielsZeilemaker