Skip to content

Comments

[AIRFLOW-XXX] Omit vendor packages from being covered by codecov#5013

Merged
Fokko merged 1 commit intoapache:masterfrom
feluelle:patch-2
Apr 8, 2019
Merged

[AIRFLOW-XXX] Omit vendor packages from being covered by codecov#5013
Fokko merged 1 commit intoapache:masterfrom
feluelle:patch-2

Conversation

@feluelle
Copy link
Member

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-XXX
    • 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:

The _vendor packages contains modules that are not directly part of airflow itself. Because of that I think we don't have to check its code coverage.

For example_dags I think there is also no need to add tests to.

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

Code Quality

  • Passes flake8

@mik-laj
Copy link
Member

mik-laj commented Mar 30, 2019

I working on tests for example dags
PolideaInternal#83
PolideaInternal#77

@feluelle
Copy link
Member Author

Oh, I see. So you think we should keep the coverage for example_dags?

BTW the automatic code ingestion in the docs looks awesome 👍

@KevinYang21
Copy link
Member

Think it is nice to include exampels if we have 100% coverage there but don't think that is strictly needed.

@Fokko
Copy link
Contributor

Fokko commented Mar 31, 2019

I know that (at least some) DAGs are used in the tests, but having coverage would be nice here. I'm not really interested in artificially increasing the code coverage, as I strongly believe it is a vanity metric.
I do think we need to exclude airflow/_vendor/*.

@feluelle
Copy link
Member Author

feluelle commented Apr 3, 2019

I think @Fokko is right actually. Moreover @mik-laj did already add tests for the example dags.

I change this PR to only exclude vendor packages.

@Fokko
Copy link
Contributor

Fokko commented Apr 5, 2019

@feluelle Did you push? I still see the exclusions of the dags folders.

@codecov-io
Copy link

codecov-io commented Apr 5, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5013      +/-   ##
==========================================
+ Coverage   76.23%   76.75%   +0.52%     
==========================================
  Files         466      450      -16     
  Lines       30101    29514     -587     
==========================================
- Hits        22948    22654     -294     
+ Misses       7153     6860     -293
Impacted Files Coverage Δ
airflow/_vendor/nvd3/scatterChart.py
airflow/_vendor/nvd3/lineChart.py
airflow/_vendor/nvd3/__init__.py
airflow/_vendor/nvd3/discreteBarChart.py
airflow/_vendor/nvd3/linePlusBarChart.py
airflow/_vendor/nvd3/pieChart.py
airflow/_vendor/slugify/slugify.py
airflow/_vendor/nvd3/multiBarChart.py
airflow/_vendor/nvd3/cumulativeLineChart.py
airflow/_vendor/nvd3/NVD3Chart.py
... and 6 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 4991c34...9a22872. Read the comment docs.

@feluelle
Copy link
Member Author

feluelle commented Apr 5, 2019

@Fokko I didn't had time. I did now (and rebased onto master).

@feluelle feluelle changed the title [AIRFLOW-XXX] Omit vendor packages and example dags from being covered by codecov [AIRFLOW-XXX] Omit vendor packages from being covered by codecov Apr 5, 2019
@Fokko
Copy link
Contributor

Fokko commented Apr 8, 2019

Thanks @feluelle

@Fokko Fokko merged commit fc3b45a into apache:master Apr 8, 2019
@feluelle feluelle deleted the patch-2 branch April 9, 2019 12:13
cthenderson pushed a commit to cthenderson/apache-airflow that referenced this pull request Apr 16, 2019
andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 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.

5 participants