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: do not import superset_config on tests #11193

Merged
merged 2 commits into from
Oct 14, 2020

Conversation

betodealmeida
Copy link
Member

SUMMARY

When running unit tests we import any superset_config.py files that are in the path, resulting in inconsistent local tests.

I modified the code so that we set an environment flag when running unit tests, and skip importing superset_config.py when the flag is set.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

NA

TEST PLAN

I had this custom configuration file:

# superset_config.py
PREVENT_UNSAFE_DB_CONNECTIONS = False

Running SQLite unit tests will then fail:

$ tox -e py38-sqlite -- tests/databases/api_tests.py::TestDatabaseApi::test_create_database_fail_sqllite
...
>       self.assertEqual(response_data, expected_response)
E       AssertionError: {'message': 'Could not connect to database.'} != {'message': {'sqlalchemy_uri': ['SQLite database cann[48 chars].']}}
E       Diff is 194 characters long. Set self.maxDiff to None to see it.

tests/databases/api_tests.py:331: AssertionError

With this PR, superset_config.py is ignored and the tests pass.

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

@codecov-io
Copy link

codecov-io commented Oct 7, 2020

Codecov Report

Merging #11193 into master will decrease coverage by 4.24%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11193      +/-   ##
==========================================
- Coverage   65.60%   61.35%   -4.25%     
==========================================
  Files         828      832       +4     
  Lines       39167    39449     +282     
  Branches     3589     3598       +9     
==========================================
- Hits        25694    24205    -1489     
- Misses      13361    15062    +1701     
- Partials      112      182      +70     
Flag Coverage Δ
#cypress ?
#javascript 62.67% <ø> (+0.36%) ⬆️
#python 60.57% <60.00%> (-0.45%) ⬇️

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

Impacted Files Coverage Δ
superset/config.py 90.11% <50.00%> (+0.03%) ⬆️
superset/utils/core.py 89.55% <66.66%> (+0.04%) ⬆️
superset/views/css_templates.py 90.00% <0.00%> (-10.00%) ⬇️
superset/db_engine_specs/presto.py 73.65% <0.00%> (-8.43%) ⬇️
superset/db_engines/hive.py 82.14% <0.00%> (-3.58%) ⬇️
superset/examples/world_bank.py 97.10% <0.00%> (-2.90%) ⬇️
superset/examples/birth_names.py 97.59% <0.00%> (-2.41%) ⬇️
superset/datasets/dao.py 86.48% <0.00%> (-1.82%) ⬇️
superset/views/alerts.py 82.97% <0.00%> (-1.44%) ⬇️
superset/models/alerts.py 96.00% <0.00%> (-0.85%) ⬇️
... and 893 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 b6728d8...0d30bc3. Read the comment docs.

@betodealmeida betodealmeida changed the title Do not import superset_config on tests fix: do not import superset_config on tests Oct 7, 2020
@betodealmeida
Copy link
Member Author

@dpgaspar do you have any idea why this test is failing? I noticed you have disabled it for Presto.

@betodealmeida
Copy link
Member Author

I created #11196 to fix the failing unit test.

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.

LGTM, but I would suggest using strtobool



def is_test() -> bool:
return "SUPERSET_TESTENV" in os.environ
Copy link
Member

Choose a reason for hiding this comment

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

Here it might be worth considering using strtobool, which will parse the string value for known string representations of boolean values so the env variable can also explicitly be set to True:

from distutils.util import strtobool 

strtobool(os.environ.get("SUPERSET_TESTENV", "false"))

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @villebro! I'll update the PR.

@betodealmeida betodealmeida merged commit 7594161 into apache:master Oct 14, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* Do not import superset_config on tests

* Use strtobool
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 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/S 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants