ci: conditionally skip python and frontend tests #12583

villebro · 2021-01-18T15:49:59Z

SUMMARY

This PR changes Python CI workflows to skip unit test execution for PRs that don't have changes that are relevant for those workflows. Here we're leveraging continue-on-error to check if files relevant to unit tests have changed, namely

Python changes:

tests/*
superset/*
requirements/*.txt
setup.py

Frontend changes:

superset_frontend/*

This is done to ensure that skipped workflows return a successful status for required checks despite not executing the expensive test steps. If any of the aforementioned file have changed, a failure exit code is triggered, which subsequent steps listen in on; if the check step returned a failure, the tests run, otherwise they are skipped.
To check changes, run check_no_file_changes.sh with the following arguments (the GITHUB_REPO and PR_NUMBER env variables need to be set):

Only check python changes: ./check_no_file_changes.sh python
Only check frontend changes: ./check_no_file_changes.sh frontend
Check python and frontend changes: ./check_no_file_changes.sh python frontend (order doesn't matter)

E2E tests have not been included in this PR, as some steps there depend in failure() which might need tweaking to run properly with this script.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Example of skipped frontend changes:

This shows the results for a PR with python changes:

Searching for changes in python files
Detected changes... Exiting with FAILURE code
bash-3.2$ PR_NUMBER=12960 GITHUB_REPO=apache/superset ./ci_check_no_file_changes.sh python
CHANGED FILES:
superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py

Searching for changes in python files
Detected changes... Exiting with FAILURE code
bash-3.2$ PR_NUMBER=12960 GITHUB_REPO=apache/superset ./ci_check_no_file_changes.sh frontend
CHANGED FILES:
superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py

Searching for changes in frontend files
No changes detected... Exiting with SUCCESS code
bash-3.2$ PR_NUMBER=12960 GITHUB_REPO=apache/superset ./ci_check_no_file_changes.sh python frontend
CHANGED FILES:
superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py

When checking for both python and frontend changes on a PR with frontend changes, the following happens:

bash-3.2$ PR_NUMBER=12920 GITHUB_REPO=apache/superset ./ci_check_no_file_changes.sh python frontend
CHANGED FILES:
superset-frontend/spec/javascripts/sqllab/TableElement_spec.jsx
superset-frontend/src/SqlLab/components/SqlEditorLeftBar.jsx
superset-frontend/src/SqlLab/components/TableElement.jsx
superset-frontend/src/SqlLab/main.less
superset-frontend/src/common/components/Collapse/Collapse.stories.tsx
superset-frontend/src/common/components/Collapse/index.tsx
superset-frontend/src/common/components/common.stories.tsx
superset-frontend/src/common/components/index.tsx
superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel.tsx
superset-frontend/src/explore/components/ControlPanelsContainer.jsx
superset-frontend/src/explore/components/DataTablesPane.tsx
superset-frontend/src/explore/components/DatasourcePanel.tsx
superset-frontend/src/explore/components/controls/FixedOrMetricControl.jsx
superset-frontend/src/explore/main.less
superset-frontend/src/views/CRUD/welcome/Welcome.tsx

Searching for changes in python files
Searching for changes in frontend files
Detected changes... Exiting with FAILURE code

TEST PLAN

Tested to correctly detect frontend and python changes locally, as well as skip tests when they are not needed to run

ADDITIONAL INFORMATION

codecov-io · 2021-01-18T16:38:37Z

Codecov Report

Merging #12583 (5e39697) into master (0f243c6) will decrease coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12583      +/-   ##
==========================================
- Coverage   66.75%   66.67%   -0.08%     
==========================================
  Files        1015     1015              
  Lines       49634    49634              
  Branches     4839     4970     +131     
==========================================
- Hits        33131    33094      -37     
- Misses      16380    16417      +37     
  Partials      123      123

Flag	Coverage Δ
cypress	`50.98% <ø> (+<0.01%)`	⬆️
javascript	`60.74% <ø> (ø)`
python	`63.83% <ø> (-0.13%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/db_engine_specs/presto.py	`73.37% <0.00%> (-8.01%)`	⬇️
superset/models/core.py	`88.58% <0.00%> (-0.28%)`	⬇️
...rontend/src/explore/components/DatasourcePanel.tsx	`90.69% <0.00%> (ø)`
...set-frontend/src/dashboard/util/getDropPosition.js	`93.65% <0.00%> (+1.58%)`	⬆️

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 0f243c6...5262a77. Read the comment docs.

amitmiran137

Wow this will reduce CI waiting time dramatically.
Thank you for that!

junlincc · 2021-01-19T18:37:03Z

@ktmud could you help review and stamp it?

ktmud · 2021-01-19T18:57:57Z

You probably wouldn't want to ignore tests/*.py for E2E because it also contains some scripts to fill examples data.

I'm also wondering what's the difference between this solution and using Github Action's built-in on.<push|pull_request>.paths config?

We can probably get away with something like this:

name: E2E

on:
  push:
    branches-ignore:
      - "dependabot/**/docs/**"
    paths-ignore:
      - "docs/**"
      - *.md
      - docker/**
      - helm/**
  pull_request:
    paths-ignore:
      - "docs/**"
      - *.md
      - docker/**
      - helm/**

# Python MySQL unit tests
name: Python MySQL

on:
  push:
    branches-ignore:
      - "dependabot/npm_and_yarn/**"
    paths:
      - superset/**
      - tests/**
      - requirements/**
      - scripts/**
      - setup.py
  pull_request:
    paths:
      - superset/**
      - tests/**
      - requirements/**
      - scripts/**
      - setup.py

nytai · 2021-01-19T20:50:11Z

@ktmud using the github syntax causes the action to forever remain in a "pending" state which block PR merges for actions that are marked required. This approach causes the action to report back ✅ even if no tests were actually run.

nytai

LGTM. The python checks only took ~20s!

ktmud · 2021-01-19T21:02:09Z

@ktmud using the github syntax causes the action to forever remain in a "pending" state which block PR merges for actions that are marked required. This approach causes the action to report back ✅ even if no tests were actually run.

Right. Totally forgot about this... We've talked about this before, lol.

ktmud · 2021-01-19T21:02:44Z

Anyway, I still don't think you can skip test/*.py for E2E tests.

ktmud · 2021-01-19T21:03:46Z

.github/workflows/test-presto.yml

+        continue-on-error: true
+        run: |
+          ./scripts/ci_check_python_changes.sh
+          ./scripts/ci_check_python_test_changes.sh


Can we maybe parameterize this script to avoid creating two largely identical files?

Anyway, I still don't think you can skip test/*.py for E2E tests.

are we? I'm not seeing any changes to superset-e2e.yml

I wanted to keep the scope of this PR as small as possible, hence didn't het add the E2E changes. Once this is merged I'll add similar logic to frontend and E2E tests.

Another benefit of parameterizing this is to allow more advanced skipping in the future---for example, we may split/skip E2E tests based on product areas.

nytai · 2021-01-19T21:05:47Z

Anyway, I still don't think you can skip test/*.py for E2E tests.

are we? I'm not seeing any changes to superset-e2e.yml

ktmud · 2021-01-19T21:18:30Z

Anyway, I still don't think you can skip test/*.py for E2E tests.

are we? I'm not seeing any changes to superset-e2e.yml

It was stated in the PR description as the reason for having two .sh files. Then maybe we don't need two files.

ktmud · 2021-01-19T21:22:55Z

BTW, we should probably also skip superset-frontend.yml for pure Python changes.

nytai · 2021-01-19T22:17:33Z

Ah, I didn't read the PR description fully. I agree, since e2e is supposed to be out final catch-all check it should probably always run. Also there is code in tests/*.py that can affect e2e test runs

villebro · 2021-01-20T07:36:11Z

Thanks for the reviews and good comments @nytai and @ktmud , I'll update the PR accordingly

villebro · 2021-01-20T07:40:18Z

BTW, we should probably also skip superset-frontend.yml for pure Python changes.

Yes, I'll do this in a follow-up PR.

dpgaspar

LGTM, just a non blocking comment, agree that E2E should run also on python changes

dpgaspar · 2021-01-20T12:02:32Z

scripts/ci_check_python_changes.sh

+EOF
+
+if [[ "${FILES}" =~ (superset\/.+\.py|setup\.py|requirements\/.+\.txt) ]]; then
+  echo "Detected changes in python files... Exiting with FAILURE code"


nit: seems logically odd that a script named check_python_changes returns "False" if there were changes, assuming it's easier to detect if: steps.check.outcome == 'failure' maybe rename the script to a negative

That's a good point also. I'll rename the files to be in line with what's being asserted.

ktmud · 2021-02-05T09:43:37Z

The ci_check_no_file_changes.sh file seems to be missing!

dpgaspar

This is great @villebro, one step forward to a faster CI

dpgaspar · 2021-02-05T13:43:38Z

scripts/ci_check_no_file_changes.sh

+# To check for python and frontend changes, run with CHECKS="python frontend"
+
+URL="https://api.github.com/repos/${GITHUB_REPO}/pulls/${PR_NUMBER}/files"
+FILES=$(curl -s -X GET -G $URL | jq -r '.[] | .filename')


nit: SC2086: Double quote to prevent globbing and word splitting.

superset-github-bot bot added the preset-io label Jan 18, 2021

pull-request-size bot added the size/M label Jan 18, 2021

villebro force-pushed the villebro/ci-skip-python branch from e44de25 to ad10911 Compare January 18, 2021 16:23

villebro force-pushed the villebro/ci-skip-python branch 2 times, most recently from 8d3f9bf to 5e39697 Compare January 18, 2021 17:55

pull-request-size bot added size/L and removed size/M labels Jan 18, 2021

villebro force-pushed the villebro/ci-skip-python branch from 5e39697 to 5262a77 Compare January 18, 2021 18:49

amitmiran137 approved these changes Jan 18, 2021

View reviewed changes

villebro changed the title ~~[WIP] ci: conditionally skip python tests~~ ci: conditionally skip python tests Jan 19, 2021

villebro requested review from ktmud, dpgaspar and nytai January 19, 2021 08:37

nytai approved these changes Jan 19, 2021

View reviewed changes

ktmud reviewed Jan 19, 2021

View reviewed changes

dpgaspar approved these changes Jan 20, 2021

View reviewed changes

villebro added 2 commits February 5, 2021 10:28

ci: conditionally skip python tests

8738570

parametrize check script and implement for frontend

6e488cc

villebro force-pushed the villebro/ci-skip-python branch from 5262a77 to 6e488cc Compare February 5, 2021 09:28

pull-request-size bot added size/M and removed size/L labels Feb 5, 2021

villebro changed the title ~~ci: conditionally skip python tests~~ ci: conditionally skip python and frontend tests Feb 5, 2021

add missing script

747b1c0

pull-request-size bot added size/L and removed size/M labels Feb 5, 2021

fix incorrect check

ab7a12e

villebro changed the title ~~ci: conditionally skip python and frontend tests~~ [WIP] ci: conditionally skip python and frontend tests Feb 5, 2021

move CHECKS to ARGS and fix typo

f10c40c

villebro force-pushed the villebro/ci-skip-python branch from 8bc148a to f10c40c Compare February 5, 2021 13:29

villebro changed the title ~~[WIP] ci: conditionally skip python and frontend tests~~ ci: conditionally skip python and frontend tests Feb 5, 2021

dpgaspar approved these changes Feb 5, 2021

View reviewed changes

fix SC2086 nit

39b47fb

villebro merged commit 5d55c43 into apache:master Feb 5, 2021

villebro deleted the villebro/ci-skip-python branch February 5, 2021 15:40

villebro mentioned this pull request Feb 6, 2021

ci: skip cypress if no code changes #12982

Merged

6 tasks

ktmud mentioned this pull request Feb 25, 2021

test: optimize codecov config #13329

Merged

6 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: conditionally skip python and frontend tests #12583

ci: conditionally skip python and frontend tests #12583

villebro commented Jan 18, 2021 •

edited

Loading

codecov-io commented Jan 18, 2021 •

edited

Loading

amitmiran137 left a comment

junlincc commented Jan 19, 2021 •

edited

Loading

ktmud commented Jan 19, 2021

nytai commented Jan 19, 2021

nytai left a comment •

edited

Loading

ktmud commented Jan 19, 2021

ktmud commented Jan 19, 2021

ktmud Jan 19, 2021

villebro Jan 20, 2021

ktmud Jan 20, 2021

nytai commented Jan 19, 2021

ktmud commented Jan 19, 2021 •

edited

Loading

ktmud commented Jan 19, 2021

nytai commented Jan 19, 2021

villebro commented Jan 20, 2021

villebro commented Jan 20, 2021

dpgaspar left a comment

dpgaspar Jan 20, 2021

villebro Jan 20, 2021

ktmud commented Feb 5, 2021

dpgaspar left a comment

dpgaspar Feb 5, 2021

ci: conditionally skip python and frontend tests #12583

ci: conditionally skip python and frontend tests #12583

Conversation

villebro commented Jan 18, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-io commented Jan 18, 2021 • edited Loading

Codecov Report

amitmiran137 left a comment

Choose a reason for hiding this comment

junlincc commented Jan 19, 2021 • edited Loading

ktmud commented Jan 19, 2021

nytai commented Jan 19, 2021

nytai left a comment • edited Loading

Choose a reason for hiding this comment

ktmud commented Jan 19, 2021

ktmud commented Jan 19, 2021

ktmud Jan 19, 2021

Choose a reason for hiding this comment

villebro Jan 20, 2021

Choose a reason for hiding this comment

ktmud Jan 20, 2021

Choose a reason for hiding this comment

nytai commented Jan 19, 2021

ktmud commented Jan 19, 2021 • edited Loading

ktmud commented Jan 19, 2021

nytai commented Jan 19, 2021

villebro commented Jan 20, 2021

villebro commented Jan 20, 2021

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Jan 20, 2021

Choose a reason for hiding this comment

villebro Jan 20, 2021

Choose a reason for hiding this comment

ktmud commented Feb 5, 2021

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Feb 5, 2021

Choose a reason for hiding this comment

villebro commented Jan 18, 2021 •

edited

Loading

codecov-io commented Jan 18, 2021 •

edited

Loading

junlincc commented Jan 19, 2021 •

edited

Loading

nytai left a comment •

edited

Loading

ktmud commented Jan 19, 2021 •

edited

Loading