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

[fix] tox running of specific tests #9097

Merged
merged 1 commit into from
Feb 7, 2020

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Feb 6, 2020

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

This PR corrects running of specific tests in tox, i.e.,

tox -e py36-mysql tests/sqllab_tests.py 

whereas previously this would have run as,

tox -e py36-mysql tests tests/sqllab_tests.py 

meaning the entire test suite was run prior to the test(s) for interest. tox provides a mechanism for default positional arguments (per here) and thus these are equivalent (the first being the default) for running the entire suite:

tox -e py36-mysql
tox -e py36-mysql tests

Note when running individual tests I ran into the RuntimeError: Working outside of application context. error during the import phase. The fix is to add the import tests.test_app import (which creates the app context) where necessary. I believe the entire test suite worked only because the first file alphabetically included the import.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

to: craig-rueda @etr2460 @mistercrunch @villebro

@john-bodley john-bodley changed the title [tox] Allowing running of specific tests [tox] Fix running of specific tests Feb 6, 2020
@john-bodley john-bodley changed the title [tox] Fix running of specific tests [fix] tox running of specific tests Feb 6, 2020
@codecov-io
Copy link

codecov-io commented Feb 6, 2020

Codecov Report

Merging #9097 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #9097      +/-   ##
=========================================
- Coverage   59.15%   59.1%   -0.06%     
=========================================
  Files         370     372       +2     
  Lines       11818   11920     +102     
  Branches     2900    2918      +18     
=========================================
+ Hits         6991    7045      +54     
- Misses       4648    4693      +45     
- Partials      179     182       +3
Impacted Files Coverage Δ
...set/assets/src/dashboard/actions/dashboardState.js 31.72% <0%> (-9.24%) ⬇️
superset/assets/src/chart/chartAction.js 43.24% <0%> (-4.71%) ⬇️
...ssets/src/dashboard/components/PropertiesModal.jsx 6.55% <0%> (-0.72%) ⬇️
...uperset/assets/src/dashboard/components/Header.jsx 41.79% <0%> (-0.28%) ⬇️
...t/assets/src/views/dashboardList/DashboardList.tsx 73.14% <0%> (-0.25%) ⬇️
...t/assets/src/dashboard/reducers/getInitialState.js 0% <0%> (ø) ⬆️
...ssets/src/dashboard/containers/DashboardHeader.jsx 100% <0%> (ø) ⬆️
superset/assets/src/welcome/App.jsx 0% <0%> (ø) ⬆️
superset/assets/src/logger/LogUtils.js 100% <0%> (ø) ⬆️
...set/assets/src/dashboard/util/setPeriodicRunner.js 66.66% <0%> (ø)
... and 1 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 916d184...8a399db. Read the comment docs.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @john-bodley , I was wondering why I wasn't able to run only a subset of tests a few weeks ago but didn't look into it. I'm sure this was working before; any idea if this is a regression from a code change or perhaps a consequence of bumping tox or similar?

Copy link
Member

@nytai nytai left a comment

Choose a reason for hiding this comment

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

👏 this was driving me crazy last week

@john-bodley john-bodley force-pushed the john-bodley--tox-nosetests branch from e481005 to 6f3b0dc Compare February 6, 2020 22:41
@pull-request-size pull-request-size bot added size/M and removed size/XS labels Feb 6, 2020
@john-bodley
Copy link
Member Author

john-bodley commented Feb 6, 2020

@villebro I believe the regression was introduced in #8418 though without the tests directory the tests don't seem to run correctly (even those nosetests is supposed to sniff out all tests) so I'm a little perplexed as to how it worked previously.

I've augmented the commit to include missing imports as running certain individual files would fail with the familiar app context error. It seems that the test suite worked merely because the first test alphabetically imported the correct file.

@john-bodley john-bodley force-pushed the john-bodley--tox-nosetests branch from 6f3b0dc to 8a399db Compare February 6, 2020 23:17
@@ -19,7 +19,7 @@ commands =
{toxinidir}/superset/bin/superset db upgrade
{toxinidir}/superset/bin/superset init
nosetests tests/load_examples_test.py
nosetests -e load_examples_test tests {posargs}
nosetests --exclude=load_examples_test {posargs:tests}
Copy link
Member Author

Choose a reason for hiding this comment

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

-e and --exclude= are equivalent, though I though --exclude was more descriptive.

@john-bodley john-bodley merged commit 8a138fb into apache:master Feb 7, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants