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-4358] Speed up test_jobs by not running tasks #5162

Merged
merged 1 commit into from
Apr 29, 2019

Conversation

ashb
Copy link
Member

@ashb ashb commented Apr 23, 2019

Make sure you have checked all steps below.

Jira

Description

  • We don't care about the behaviour of the tasks (we test those elsewhere) - we just care that the task is "run" and yeilds a state, which we can all do more directly.

    On my laptop this takes the time for SchedulerJobTest from 133s to 35s, and reduces the BackfillJobTests from >5mins to mere seconds!

    I think this approach still tests the behaviour well enough

Tests

  • Changed tests to check events that are run via executor, but it won't run it

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

@feng-tao
Copy link
Member

Nice! It is always good to speed up the CI!

@ashb
Copy link
Member Author

ashb commented Apr 23, 2019

Looks like I might have some ordering in the tests to sort out. Sorted.

@ashb ashb force-pushed the stub-executor-tests branch 4 times, most recently from e1e560c to e6fb983 Compare April 24, 2019 10:54
@ashb
Copy link
Member Author

ashb commented Apr 24, 2019

Imprecise numbers on tests - it looks like this saves 4mins on Sqlite tests, and ~10mins on mysql/postgres (where we run backfill!)

Before (master build):

TOX_ENV=py35-backend_mysql-env_docker 34 min 24 sec
TOX_ENV=py35-backend_sqlite-env_docker 25 min 37 sec
TOX_ENV=py35-backend_postgres-env_docker 35 min 52 sec
TOX_ENV=py35-backend_postgres-env_kubernetes KUBERNETES_VERSION=v1.13.0 28 min 11 sec

After:

TOX_ENV=py35-backend_mysql-env_docker 24 min 51 sec
TOX_ENV=py35-backend_sqlite-env_docker 21 min 28 sec
TOX_ENV=py35-backend_postgres-env_docker PYTHON_VERSION=3 26 min 19 sec
TOX_ENV=py35-backend_postgres-env_kubernetes KUBERNETES_VERSION=v1.13.0 27 min 33 sec

@codecov-io
Copy link

codecov-io commented Apr 24, 2019

Codecov Report

Merging #5162 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5162      +/-   ##
==========================================
- Coverage   78.56%   78.56%   -0.01%     
==========================================
  Files         469      469              
  Lines       29912    29912              
==========================================
- Hits        23501    23500       -1     
- Misses       6411     6412       +1
Impacted Files Coverage Δ
airflow/jobs.py 78.49% <0%> (-0.18%) ⬇️
airflow/utils/dag_processing.py 59.47% <0%> (+0.17%) ⬆️

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 d63b6c9...902fc34. Read the comment docs.

@ashb ashb requested a review from potiuk April 24, 2019 14:04
@feng-tao
Copy link
Member

LGTM

@potiuk
Copy link
Member

potiuk commented Apr 26, 2019

@ashb -> i will have more time on weekend to take a look. But at first glance it looks good.

@ashb ashb requested review from XD-DENG and feng-tao April 26, 2019 19:50
airflow/utils/db.py Outdated Show resolved Hide resolved
Copy link
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

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

Great work @ashb

I only found one thing I think can be improved here..

tests/executors/test_executor.py Show resolved Hide resolved
@ashb ashb force-pushed the stub-executor-tests branch 2 times, most recently from f9d7a5c to af613ea Compare April 29, 2019 11:55
tests/test_jobs.py Outdated Show resolved Hide resolved
@ashb ashb force-pushed the stub-executor-tests branch 2 times, most recently from ba2885c to 8774cee Compare April 29, 2019 16:20
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.

I like this a lot!

tests/executors/test_executor.py Show resolved Hide resolved
tests/executors/test_executor.py Outdated Show resolved Hide resolved
We don't care about the behaviour of the tasks in most cases (as we test
those elsewhere) - we just care that the task is "run" and ends in a
state, which we can all do more directly.

On my laptop this takes the time for SchedulerJobTest from 133s to 35s,
and reduces the BackfillJobTests from >5mins to mere seconds!
Copy link
Member

@mik-laj mik-laj left a comment

Choose a reason for hiding this comment

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

LGTM. :-)

@ashb ashb merged commit b69c686 into apache:master Apr 29, 2019
@ashb ashb deleted the stub-executor-tests branch April 29, 2019 21:54
potiuk pushed a commit that referenced this pull request Apr 30, 2019
We don't care about the behaviour of the tasks in most cases (as we test
those elsewhere) - we just care that the task is "run" and ends in a
state, which we can all do more directly.

On my laptop this takes the time for SchedulerJobTest from 133s to 35s,
and reduces the BackfillJobTests from >5mins to mere seconds!

(cherry picked from commit b69c686)
potiuk pushed a commit that referenced this pull request Apr 30, 2019
We don't care about the behaviour of the tasks in most cases (as we test
those elsewhere) - we just care that the task is "run" and ends in a
state, which we can all do more directly.

On my laptop this takes the time for SchedulerJobTest from 133s to 35s,
and reduces the BackfillJobTests from >5mins to mere seconds!

(cherry picked from commit b69c686)
andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
We don't care about the behaviour of the tasks in most cases (as we test
those elsewhere) - we just care that the task is "run" and ends in a
state, which we can all do more directly.

On my laptop this takes the time for SchedulerJobTest from 133s to 35s,
and reduces the BackfillJobTests from >5mins to mere seconds!
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
We don't care about the behaviour of the tasks in most cases (as we test
those elsewhere) - we just care that the task is "run" and ends in a
state, which we can all do more directly.

On my laptop this takes the time for SchedulerJobTest from 133s to 35s,
and reduces the BackfillJobTests from >5mins to mere seconds!
dharamsk pushed a commit to postmates/airflow that referenced this pull request Aug 8, 2019
We don't care about the behaviour of the tasks in most cases (as we test
those elsewhere) - we just care that the task is "run" and ends in a
state, which we can all do more directly.

On my laptop this takes the time for SchedulerJobTest from 133s to 35s,
and reduces the BackfillJobTests from >5mins to mere seconds!

(cherry picked from commit b69c686)
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

6 participants