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-4058] Rename models tests #4901

Merged
merged 3 commits into from
Mar 11, 2019
Merged

Conversation

ffinfo
Copy link
Contributor

@ffinfo ffinfo commented Mar 11, 2019

Make sure you have checked all steps below.

Jira

Description

Renaming models.py to test_models.py

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.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
    • 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

Code Quality

  • Passes flake8

@codecov-io
Copy link

Codecov Report

Merging #4901 into master will increase coverage by 0.22%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4901      +/-   ##
==========================================
+ Coverage    75.3%   75.53%   +0.22%     
==========================================
  Files         450      450              
  Lines       29034    29034              
==========================================
+ Hits        21865    21931      +66     
+ Misses       7169     7103      -66
Impacted Files Coverage Δ
airflow/contrib/operators/ssh_operator.py 83.54% <0%> (+1.26%) ⬆️
airflow/hooks/dbapi_hook.py 87.9% <0%> (+8.87%) ⬆️
airflow/utils/db.py 90.38% <0%> (+51.92%) ⬆️

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 f8a24aa...fd36d38. Read the comment docs.

@ashb ashb merged commit 4fb91b0 into apache:master Mar 11, 2019
@ashb
Copy link
Member

ashb commented Mar 13, 2019

@ffinfo It seems that since we merged this commit something has happened to our logging config in tests, and we now see everything at debug level in the logs (example https://travis-ci.org/apache/airflow/jobs/504637817 compared to the build on master just before https://travis-ci.org/apache/airflow/jobs/504628389) which makes it even harder to find failures :(

I can't see anything in the diff that might cause this, but maybe there was some side-effect from somewhere else? Would you have a spare hour to try and look at what is causing it?

@ffinfo
Copy link
Contributor Author

ffinfo commented Mar 13, 2019

@ashb I did do some tracing but I think the cause is not in this merge request.

I checked my own fork on travis. Here on the same commit it did not happen:
https://travis-ci.org/ffinfo/airflow/jobs/504557217

The only difference between the build is that travis did a auto rebase on master before build, see also:
0fd8da0
(I did not generate this commit, travis does this automated.

This means that between my latest rebase/merge and the point where I did create the PR there was an other change on master that did caused this.

I will try to back trace this to the commit on master that did cause this. And make a PR to fix this.

@ashb
Copy link
Member

ashb commented Mar 13, 2019

Thanks for taking the time! @XD-DENG tracked it to this commit on master, but we couldn't see anything obvious that might cause it either.

ashb pushed a commit to ashb/airflow that referenced this pull request Apr 1, 2019
…pache#4901)

We had `from .models import *` inside test/__init__.py as a kludge around this test
file not being named according to expected Python conventions. Renaming the file
makes more test tools happier (and makes it easier to run a single test file without
importing half of the test tree which the current approach suffers from)
andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
…pache#4901)

We had `from .models import *` inside test/__init__.py as a kludge around this test
file not being named according to expected Python conventions. Renaming the file
makes more test tools happier (and makes it easier to run a single test file without
importing half of the test tree which the current approach suffers from)
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
…pache#4901)

We had `from .models import *` inside test/__init__.py as a kludge around this test
file not being named according to expected Python conventions. Renaming the file
makes more test tools happier (and makes it easier to run a single test file without
importing half of the test tree which the current approach suffers from)
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.

3 participants