Skip to content

[AIRFLOW-5895] Move HDFS stuff from tests/core.py#6544

Merged
mik-laj merged 4 commits intoapache:masterfrom
PolideaInternal:AIRFLOW-5895
Nov 12, 2019
Merged

[AIRFLOW-5895] Move HDFS stuff from tests/core.py#6544
mik-laj merged 4 commits intoapache:masterfrom
PolideaInternal:AIRFLOW-5895

Conversation

@mik-laj
Copy link
Member

@mik-laj mik-laj commented Nov 11, 2019

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"
    • https://issues.apache.org/jira/browse/AIRFLOW-5895
    • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
    • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
    • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

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

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

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.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Nice!

@potiuk
Copy link
Member

potiuk commented Nov 11, 2019

Travis is sad here as well. I recommend to run pre-commit hooks every time @mik-laj

@mik-laj
Copy link
Member Author

mik-laj commented Nov 12, 2019

@potiuk Every time I use pre-commit, it causes wrong file changes. I have to change them to make Travis happy. I do not know the reason for this problem, so I had to turn it off to be able to continue working on this project.
For example:

@@ -41,6 +41,8 @@ from dateutil.relativedelta import relativedelta
 from numpy.testing import assert_array_almost_equal
 from pendulum import utcnow

+from tests.test_utils.config import conf_vars
+
 from airflow import DAG, configuration, exceptions, jobs, models, settings, utils
 from airflow.bin import cli
 from airflow.configuration import AirflowConfigException, conf, run_command
@@ -62,7 +64,6 @@ from airflow.utils.dates import days_ago, infer_time_unit, round_time, scale_tim
 from airflow.utils.state import State
 from airflow.utils.timezone import datetime
 from airflow.version import version
-from tests.test_utils.config import conf_vars

@potiuk
Copy link
Member

potiuk commented Nov 12, 2019

I am sure it's not every time. It never ever happened to me. I am super surprised because exactly the same check is run on Travis as in your local environment.

I suggest you enable it and let me know when it happens again. I am happy to take a look when it happens again and help to find the reason (if this is really so frequent). You are the only person I know who has this problem so it would be worth checking what's wrong in your setup and prevent it happening also for others.

If you look now at Travis - there is an over-indent in test_hdfs which would have been detected locally if you have pre-commits enabled. That's the whole point of pre-commit.

Re. Import above - can you please explain what was wrong? The change above is not pre-commit - but simply isort in the works. What's wrong about this? The isort sorting algorithm is such that the "airflow" (my) imports are always at the end. We all agreed we use isort so we should follow it.
Was it failing in Travis static checks or unit tests? If the latter and there is a good reason to keep sorting, then it should be as simple as disabling isort for the affected line or even for the whole file: https://isort.readthedocs.io/en/latest/#skip-processing-of-imports-outside-of-configuration

Or we can configure skip rules in setup.cfg

Let me know if any of those help. And happy to help further.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's worth to consider to make tests.test_utils.mocks to keep there all fake / mock stuff?

Copy link
Member Author

Choose a reason for hiding this comment

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

If there are a large number of them, we can create such a structure. Now it would only complicate the situation/import.

Copy link
Member Author

Choose a reason for hiding this comment

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

If someone wants to add more things to HDFS then they will have ready place.

@codecov-io
Copy link

codecov-io commented Nov 12, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@6b8dc40). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6544   +/-   ##
=========================================
  Coverage          ?   83.28%           
=========================================
  Files             ?      645           
  Lines             ?    37286           
  Branches          ?        0           
=========================================
  Hits              ?    31052           
  Misses            ?     6234           
  Partials          ?        0

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 6b8dc40...d2f1b64. Read the comment docs.

@potiuk
Copy link
Member

potiuk commented Nov 12, 2019

@mik-laj -> happy we could solve your isort configuration (#6544 (comment)) - for anyone having similar problems and finding this thread : .isort.cfg in your home directory can override setting of setup.cfg [isort] section.

@mik-laj mik-laj merged commit c481d70 into apache:master Nov 12, 2019
GnunuX pushed a commit to GnunuX/airflow that referenced this pull request Nov 13, 2019
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.

4 participants