Skip to content

Ci speedup #845

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.

Sign up for GitHub

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

Merged
byronz merged 5 commits into from
Jul 31, 2019
Merged

Ci speedup #845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e785233
:camera_flash: make percy setup suitable for CI parallelism
Jul 31, 2019
8a71695
Jul 31, 2019
e6566c2
Jul 31, 2019
adea818
Jul 31, 2019
eb8027e
:pencil2: update contribute to fix some inconsistency
Jul 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
241 changes: 169 additions & 72 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,121 +1,218 @@
version: 2

jobs:
"percy-finalize":
docker:
- image: percyio/agent
steps:
- run: percy finalize --all
Copy link
Contributor Author

@byronz byronz Jul 31, 2019

Choose a reason for hiding this comment

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

this works together with a PERCY_PARALLEL_TOTAL: '-1' in test job, percy command will compare the number of containers by fetching ENV variable value from circleci.


"python-2.7": &test-template
"lint-unit-37": &lint-unit
working_directory: ~/dash
docker:
- image: circleci/python:2.7-stretch-node-browsers
- image: circleci/python:3.7-stretch-node-browsers
environment:
PYLINTRC: .pylintrc
PYLINTRC: .pylintrc37
PYVERSION: python37

steps:
- checkout

- run:
name: ℹ️ CI Context
command: |
echo "TRIGGERER: ${CIRCLE_USERNAME}"
echo "BUILD_NUMBER: ${CIRCLE_BUILD_NUM}"
echo "BUILD_URL: ${CIRCLE_BUILD_URL}"
echo "BRANCH: ${CIRCLE_BRANCH}"
echo "RUNNING JOB: ${CIRCLE_JOB}"
echo "JOB PARALLELISM: ${CIRCLE_NODE_TOTAL}"
echo "CIRCLE_REPOSITORY_URL: ${CIRCLE_REPOSITORY_URL}"
echo $CIRCLE_JOB > circlejob.txt

- run: echo $PYVERSION > ver.txt
- restore_cache:
key: v-{{ checksum "circlejob.txt" }}-{{ checksum "requires-ci.txt" }}-{{ checksum "requires-install.txt" }}-{{ checksum "requires-testing.txt" }}
key: dep-{{ checksum "ver.txt" }}-{{ checksum "requires-ci.txt" }}-{{ checksum "requires-install.txt" }}-{{ checksum "requires-testing.txt" }}
Copy link
Contributor Author

@byronz byronz Jul 31, 2019

Choose a reason for hiding this comment

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

the cache key formula might need further tuning later

- run:
name: 🚧 pip dev requirements
command: |
sudo pip install --upgrade virtualenv
python -m venv venv || virtualenv venv
. venv/bin/activate
pip install -r requires-install.txt -r requires-ci.txt -r requires-testing.txt --quiet
python -m venv venv || virtualenv venv && . venv/bin/activate
pip install -e . -r requires-install.txt -r requires-ci.txt -r requires-testing.txt --quiet
- save_cache:
key: v-{{ checksum "circlejob.txt" }}-{{ checksum "requires-ci.txt" }}-{{ checksum "requires-install.txt" }}-{{ checksum "requires-testing.txt" }}
key: dep-{{ checksum "ver.txt" }}-{{ checksum "requires-ci.txt" }}-{{ checksum "requires-install.txt" }}-{{ checksum "requires-testing.txt" }}
paths:
- "venv"

- run:
name: 🌸 linting
name: 🌸 Python & JS Lint
command: |
. venv/bin/activate
pip install -e .[ci,testing] --quiet
pip list | grep dash
flake8 dash setup.py
flake8 --ignore=E123,E126,E501,E722,E731,F401,F841,W503,W504 --exclude=metadata_test.py tests
pip install -e . --quiet && pip list | grep dash
flake8 dash setup.py && flake8 --ignore=E123,E126,E501,E722,E731,F401,F841,W503,W504 --exclude=metadata_test.py tests
pylint dash setup.py --rcfile=$PYLINTRC
pylint tests/unit tests/integration/devtools tests/integration/renderer tests/integration/dash_assets -d all -e C0410,C0411,C0412,C0413,W0109
cd dash-renderer && npm install --ignore-scripts && npm run lint:test && npm run format:test

- run:
name: ⛑ Run unit tests
name: ⛑ Unit Tests
command: |
. venv/bin/activate
mkdir test-reports
PYTHONPATH=~/dash/tests/assets pytest --junitxml=test-reports/junit.xml tests/unit
- store_test_results:
path: test-reports
- store_artifacts:
path: test-reports
PYTHONPATH=~/dash/tests/assets pytest tests/unit

"lint-unit-36":
<<: *lint-unit
docker:
- image: circleci/python:3.6-stretch-node-browsers
environment:
PYLINTRC: .pylintrc
PYVERSION: python36

"lint-unit-27":
<<: *lint-unit
docker:
- image: circleci/python:2.7-stretch-node-browsers
environment:
PYLINTRC: .pylintrc
PYVERSION: python27

"build-core-37": &build-core
working_directory: ~/dash
docker:
- image: circleci/python:3.7-stretch-node-browsers
environment:
PYVERSION: python37
steps:
- checkout
- run: echo $PYVERSION > ver.txt
- restore_cache:
key: dep-{{ checksum "ver.txt" }}-{{ checksum "requires-ci.txt" }}-{{ checksum "requires-install.txt" }}-{{ checksum "requires-testing.txt" }}
- run:
name: 🚧 build renderer
name: 🚧 build core
command: |
. venv/bin/activate
cd dash-renderer
npm install --ignore-scripts && npm run build:dev && npm run build
pip install -e . --quiet
. venv/bin/activate && pip install --upgrade -e . --quiet && mkdir packages
python setup.py sdist && mv dist/* packages/
cd dash-renderer && npm install --ignore-scripts && npm run build:dev && npm run build && python setup.py sdist && mv dist/* ../packages/ && cd ..
git clone --depth 1 https://github.com/plotly/dash-core-components.git
cd dash-core-components && npm install --ignore-scripts && npm run build && python setup.py sdist && mv dist/* ../packages/ && cd ..
git clone --depth 1 https://github.com/plotly/dash-renderer-test-components
cd dash-renderer-test-components && npm install --ignore-scripts && npm run build:all && python setup.py sdist && mv dist/* ../packages/ && cd ..
- persist_to_workspace:
root: ~/dash
paths:
- packages

"build-core-36":
<<: *build-core
docker:
- image: circleci/python:3.6-stretch-node-browsers
environment:
PYVERSION: python36

"build-core-27":
<<: *build-core
docker:
- image: circleci/python:2.7-stretch-node-browsers
environment:
PYVERSION: python27

"build-misc-37": &build-misc
working_directory: ~/dash
docker:
- image: circleci/python:3.7-stretch-node-browsers
environment:
PYVERSION: python37

steps:
- checkout
- run: echo $PYVERSION > ver.txt
- restore_cache:
key: dep-{{ checksum "ver.txt" }}-{{ checksum "requires-ci.txt" }}-{{ checksum "requires-install.txt" }}-{{ checksum "requires-testing.txt" }}
- run:
name: 🚧 install dependencies from latest master commit
name: 🚧 build misc
command: |
git clone --depth 1 https://github.com/plotly/dash-core-components.git
git clone --depth 1 https://github.com/plotly/dash-html-components.git
. venv/bin/activate && pip install --upgrade -e . --quiet && mkdir packages
git clone --depth 1 https://github.com/plotly/dash-table.git
git clone --depth 1 https://github.com/plotly/dash-renderer-test-components
. venv/bin/activate
cd dash-core-components && npm install --ignore-scripts && npm run build && pip install -e . && cd ..
cd dash-html-components && npm install --ignore-scripts && npm run build && pip install -e . && cd ..
cd dash-table && npm install --ignore-scripts && npm run build && pip install -e . && cd ..
cd dash-renderer-test-components && npm install --ignore-scripts && npm run build:all && pip install -e . && cd ..
cd dash-table && npm install --ignore-scripts && npm run build && python setup.py sdist && mv dist/* ../packages/ && cd ..
git clone --depth 1 https://github.com/plotly/dash-html-components.git
cd dash-html-components && npm install --ignore-scripts && npm run build && python setup.py sdist && mv dist/* ../packages/ && cd ..
- persist_to_workspace:
root: ~/dash
paths:
- packages

"build-misc-36":
<<: *build-misc
docker:
- image: circleci/python:3.6-stretch-node-browsers
environment:
PYVERSION: python36

"build-misc-27":
<<: *build-misc
docker:
- image: circleci/python:2.7-stretch-node-browsers
environment:
PYVERSION: python27

"test-37": &test
working_directory: ~/dash
docker:
- image: circleci/python:3.7-stretch-node-browsers
environment:
PERCY_PARALLEL_TOTAL: '-1'
PYVERSION: python37
parallelism: 3
steps:
- checkout
- run: echo $PYVERSION > ver.txt
- restore_cache:
key: dep-{{ checksum "ver.txt" }}-{{ checksum "requires-ci.txt" }}-{{ checksum "requires-install.txt" }}-{{ checksum "requires-testing.txt" }}
- attach_workspace:
at: ~/dash
- run:
name: ⚙️ run integration test
name: ⚙️ Run Integration Tests
command: |
. venv/bin/activate
pytest --junitxml=test-reports/junit_intg.xml tests/integration/
cd packages && ls -la && pip install * && cd .. && pip list | grep dash
TESTFILES=$(circleci tests glob "tests/integration/**/test_*.py" | circleci tests split --split-by=timings)
Copy link
Contributor Author

@byronz byronz Jul 31, 2019

Choose a reason for hiding this comment

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

circleci way to split and select tests based on previous execution duration infos

pytest --headless --durations=10 --junitxml=test-reports/junit_intg.xml ${TESTFILES}
- store_artifacts:
path: test-reports
- store_test_results:
path: test-reports
- store_artifacts:
path: /tmp/dash_artifacts

"test-36":
<<: *test
docker:
- image: circleci/python:3.6-stretch-node-browsers
environment:
PERCY_ENABLE: '0'
PYVERSION: python36

"python-3.6":
<<: *test-template
docker:
- image: circleci/python:3.6-stretch-node-browsers
environment:
PYLINTRC: .pylintrc
PERCY_ENABLE: 0

"python-3.7":
<<: *test-template
docker:
- image: circleci/python:3.7-stretch-node-browsers
environment:
PYLINTRC: .pylintrc37
PERCY_ENABLE: 0

"test-27":
<<: *test
docker:
- image: circleci/python:2.7-stretch-node-browsers
environment:
PERCY_ENABLE: '0'
PYVERSION: python27

workflows:
version: 2
build:
python3.7:
jobs:
- lint-unit-37
- build-core-37
- build-misc-37
- "test-37":
requires:
- build-core-37
- build-misc-37
- "percy-finalize":
requires:
- test-37
python3.6:
jobs:
- lint-unit-36
- build-core-36
- build-misc-36
- "test-36":
requires:
- build-core-36
- build-misc-36
python2.7:
jobs:
- "python-2.7"
- "python-3.6"
- "python-3.7"
- lint-unit-27
- build-core-27
- build-misc-27
- "test-27":
requires:
- build-core-27
- build-misc-27
4 changes: 2 additions & 2 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ $ pip install -r .circleci/requirements/dev-requirements.txt
```
## Git

Use the [GitHub flow][] when proposing contributions to this repository (i.e. create a feature branch and submit a PR against the **dev** branch).
Use the [GitHub flow][] when proposing contributions to this repository (i.e. create a feature branch and submit a PR against the default branch).

### Organize your commits

Expand Down Expand Up @@ -63,7 +63,7 @@ Emojis make the commit messages :cherry_blossom:. If you have no idea about what

We use both `flake8` and `pylint` for basic linting check, please refer to the relevant steps in `.circleci/config.yml`.

Note that we also start using [`black`](https://black.readthedocs.io/en/stable/) as formatter during the test code migration.
Note that we also start using [`black`](https://black.readthedocs.io/en/stable/) as formatter during the test code migration.

## Tests

Expand Down
3 changes: 0 additions & 3 deletions dash/testing/browser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,8 @@ def __enter__(self):
def __exit__(self, exc_type, exc_val, traceback):
try:
self.driver.quit()
self.percy_runner.finalize_build()
except WebDriverException:
logger.exception("webdriver quit was not successful")
except percy.errors.Error:
logger.exception("percy runner failed to finalize properly")

def percy_snapshot(self, name=""):
"""percy_snapshot - visual test api shortcut to `percy_runner.snapshot`
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/test_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,7 +680,7 @@ def update_output(value):
assert dash_duo.find_element("#output-pre").text == "request_pre!!!"
assert dash_duo.find_element("#output-post").text == "request_post ran!"

dash_duo.percy_snapshot(name="request-hooks")
dash_duo.percy_snapshot(name="request-hooks intg")


def test_inin015_with_custom_renderer_interpolated(dash_duo):
Expand Down
6 changes: 3 additions & 3 deletions tests/integration/test_race_conditions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
import time
import flask

from dash import Dash
from dash.dependencies import Input, Output
import dash_html_components as html
import dash_core_components as dcc
from dash import Dash
from dash.dependencies import Input, Output

from .IntegrationTests import IntegrationTests
from .utils import wait_for
Expand All @@ -16,7 +16,7 @@ def setUp(self):
pass


DELAY_TIME = 1
DELAY_TIME = 0.2
Copy link
Collaborator

@alexcjohnson alexcjohnson Jul 31, 2019

Choose a reason for hiding this comment

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

heh that in itself should be a huge speedup. Should be just fine - if for some reason that's ever not a long enough delay to actually have the responses arrive at the front end in the desired order, it would be a (presumably rare) missed test, or testing a different order, but one that should still be valid. So I'm happy to make this change, but some day we should refactor this test to something a little smarter that actually triggers the responses based on evidence that the previous response has been received and acted upon, rather than just time.



def create_race_conditions_test(endpoints):
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/test_render.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,7 +957,7 @@ def update_output(value):
self.wait_for_text_to_equal('#output-post', 'request_post changed this text!')
self.wait_for_text_to_equal('#output-post-payload', '{"output":"output-1.children","changedPropIds":["input.value"],"inputs":[{"id":"input","property":"value","value":"fire request hooks"}]}')
self.wait_for_text_to_equal('#output-post-response', '{"props":{"children":"fire request hooks"}}')
self.percy_snapshot(name='request-hooks')
self.percy_snapshot(name='request-hooks render')
Copy link
Collaborator

@alexcjohnson alexcjohnson Jul 31, 2019

Choose a reason for hiding this comment

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

oh interesting - was one of these snapshots blocking the other one previously? How did you discover this? Do we need some additional mechanism (like a list of previously-used names inside percy_snapshot - though I guess in this case that wouldn't help as one's the new framework and the other is still using the old system) to ensure we don't double up a name again?

Copy link
Collaborator

@alexcjohnson alexcjohnson Jul 31, 2019

Choose a reason for hiding this comment

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

And I guess the next question: do these two tests actually both have distinct value, or should we delete one of them?

Copy link
Contributor Author

@byronz byronz Jul 31, 2019

Choose a reason for hiding this comment

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

I got a 400 error when the test job start to have parallelism > 1

Copy link
Collaborator

@alexcjohnson alexcjohnson Jul 31, 2019

Choose a reason for hiding this comment

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

oh weird - so it allows multiple snapshots with the same name from the same process but not from different processes? very strange, though useful I guess as any test we do in our own code could only catch duplicates in the same process.

Copy link
Contributor Author

@byronz byronz Jul 31, 2019

Choose a reason for hiding this comment

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

there was another issue with the previous approach, as we spin up a browser per test vs per session, the percy_finalize was called after each test case.

anyway, there are some extra integration under the hood from percy client, it adds some extra protection for all major CI tools.

Copy link
Collaborator

@alexcjohnson alexcjohnson Jul 31, 2019

Choose a reason for hiding this comment

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

Ah, got it - right, it's probably the extra finalizing that buried the duplicate, and now percy will be taking care of ensuring uniqueness.


def test_graphs_in_tabs_do_not_share_state(self):
app = dash.Dash()
Expand Down