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

[AIRFLOW-2860] Update tests for druid hook #3710

Merged
merged 2 commits into from
Aug 7, 2018

Conversation

awelsh93
Copy link
Contributor

@awelsh93 awelsh93 commented Aug 6, 2018

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"

Description

  • Here are some details about my PR, including screenshots of any UI changes:

This pull request updates the tests for airflow/hooks/druid_hook.py and also adds an assert so that timeout must be greater than or equal to 1 second. This should fix the failing build in #3707.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    I ran nosetests /tests/hooks/test_druid_hook.py

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

- Also assert that timeout is >= 1
@codecov-io
Copy link

codecov-io commented Aug 6, 2018

Codecov Report

Merging #3710 into master will increase coverage by 59.86%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3710       +/-   ##
===========================================
+ Coverage   17.69%   77.56%   +59.86%     
===========================================
  Files         204      204               
  Lines       15766    15768        +2     
===========================================
+ Hits         2790    12230     +9440     
+ Misses      12976     3538     -9438
Impacted Files Coverage Δ
airflow/hooks/druid_hook.py 87.67% <50%> (+87.67%) ⬆️
airflow/utils/operator_resources.py 86.95% <0%> (+4.34%) ⬆️
airflow/settings.py 79.33% <0%> (+4.95%) ⬆️
airflow/executors/__init__.py 63.46% <0%> (+5.76%) ⬆️
airflow/utils/sqlalchemy.py 73.91% <0%> (+10.86%) ⬆️
airflow/utils/decorators.py 91.66% <0%> (+14.58%) ⬆️
airflow/__init__.py 80.43% <0%> (+15.21%) ⬆️
airflow/hooks/oracle_hook.py 15.47% <0%> (+15.47%) ⬆️
airflow/task/task_runner/__init__.py 63.63% <0%> (+18.18%) ⬆️
airflow/utils/db.py 33.33% <0%> (+18.25%) ⬆️
... and 162 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d12aacd...1a8abf1. Read the comment docs.

@@ -53,6 +54,8 @@ def __init__(
self.max_ingestion_time = max_ingestion_time
self.header = {'content-type': 'application/json'}

assert self.timeout >= 1
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not to put assert outside of test code. Could you change it to throw exception when timeout is less than 1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also please use self.assertXXX otherwise we won't see causes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Strike that, missed the location. I would handle this as a ValueError though

Copy link
Contributor

Choose a reason for hiding this comment

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

@awelsh93
Can you change this line to:

if self.timeout < 1:
   raise ValueError("Druid timeout should be equal or greater than 1")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - thanks

@bolkedebruin
Copy link
Contributor

I think this pr is blocking (as in it is required for) #3708 I cannot get beyond the submit timeout test.

@bolkedebruin bolkedebruin merged commit 4f138df into apache:master Aug 7, 2018
@awelsh93
Copy link
Contributor Author

awelsh93 commented Aug 7, 2018

Thanks for merging this - can the change in #3707 get another review please? It was merged but then reverted due to tests failing which have now been fixed in this pull request.

@awelsh93 awelsh93 deleted the AIRFLOW-2860-FIX branch August 14, 2018 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants