test: birth names #12226

kstrz · 2020-12-30T11:42:48Z

SUMMARY

Replace birth names example with fixtures for testing.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

All tests should pass.

ADDITIONAL INFORMATION

kstrz · 2020-12-30T14:05:03Z

Hey! Could you look at it? @willbarrett @dpgaspar

willbarrett

Overall looks good - one comment on teardown, then this needs a rebase.

willbarrett · 2021-01-04T23:02:38Z

tests/celery_tests.py

@@ -208,7 +214,10 @@ def test_run_sync_query_cta_config(setup_sqllab, ctas_method):
    results = run_sql(query.select_sql)
    assert QueryStatus.SUCCESS == results["status"], result

+    db.get_engine().execute(f"DROP {ctas_method} {CTAS_SCHEMA_NAME}.{tmp_table_name}")


Should this be refactored into a shared teardown method?

dpgaspar

Looks good, but has a conflict and left a couple of comments (most non blocking for sure)

dpgaspar · 2021-01-05T09:13:47Z

superset/examples/birth_names.py

+    create_dashboard(slices)
+
+
+def _set_table_metadata(obj: "BaseDatasource", database: "Database") -> None:


nit: obj: "BaseDatasource" is called tbl: "BaseDatasource" on create_slices we could name them equally maybe datasource?

dpgaspar · 2021-01-05T09:14:03Z

superset/examples/birth_names.py

    obj.database = database
    obj.filter_select_enabled = True
    obj.fetch_metadata()

+
+def _add_table_metrics(obj: "BaseDatasource") -> None:


nit: same here

dpgaspar · 2021-01-05T09:14:32Z

superset/examples/birth_names.py

@@ -161,7 +169,7 @@ def create_slices(tbl: BaseDatasource) -> Tuple[List[Slice], List[Slice]]:
    }

    slice_props = dict(
-        datasource_id=tbl.id, datasource_type="table", owners=[admin], created_by=admin
+        datasource_id=tbl.id, datasource_type="table", owners=[], created_by=admin


dpgaspar · 2021-01-05T09:25:47Z

tests/charts/api_tests.py

@@ -435,6 +436,11 @@ def test_create_chart(self):
        """
        Chart API: Test create chart
        """
+        dashes = (
+            db.session.query(Dashboard)


nit: we can just select the id's here db.session.query(Dashboard.id)

dpgaspar · 2021-01-05T09:27:56Z

tests/conftest.py

@@ -43,14 +43,12 @@ def setup_sample_data() -> Any:

        examples.load_css_templates()
        examples.load_world_bank_health_n_pop(sample=True)
-        examples.load_birth_names(sample=True)

    yield

    with app.app_context():
        engine = get_example_database().get_sqla_engine()
        engine.execute("DROP TABLE wb_health_population")


Should we use IF EXISTS here also?

Do you mean leaving sth like this: engine.execute("DROP TABLE IF EXISTS birth_names")?

dpgaspar · 2021-01-05T09:29:41Z

tests/core_tests.py

@@ -730,6 +747,7 @@ def test_fetch_datasource_metadata(self):
        for k in keys:
            self.assertIn(k, resp.keys())

+    @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")


If this is replicated on almost every method, can we use the fixture at the module level?

I think it's better to load fresh data for each test because some tests may modify data a bit. Using module fixture may result in creating some flaky tests.

True, will probably load fast enough also

dpgaspar · 2021-01-05T09:31:16Z

tests/dashboards/api_tests.py

@@ -1147,7 +1152,12 @@ def test_export_bundle(self):
        """
        Dashboard API: Test dashboard export
        """
-        argument = [1, 2]
+        dashes = (


nit: would be nice to refactor and make this DRY

dpgaspar · 2021-01-05T09:32:27Z

tests/datasets/commands_tests.py

@@ -234,7 +234,8 @@ def test_import_v0_dataset_cli_export(self):
        assert len(dataset.metrics) == 2
        assert dataset.main_dttm_col == "ds"
        assert dataset.filter_select_enabled
-        assert [col.column_name for col in dataset.columns] == [
+        dataset.columns.sort(key=lambda obj: obj.column_name)


nice, this was probably flaky

dpgaspar

LGTM after conflicts and CI passes

codecov-io · 2021-01-07T07:27:07Z

Codecov Report

Merging #12226 (09cfb37) into master (64b4977) will decrease coverage by 3.40%.
The diff coverage is 76.00%.

@@            Coverage Diff             @@
##           master   #12226      +/-   ##
==========================================
- Coverage   66.15%   62.74%   -3.41%     
==========================================
  Files        1014     1014              
  Lines       49510    49506       -4     
  Branches     5063     5077      +14     
==========================================
- Hits        32753    31063    -1690     
- Misses      16607    18233    +1626     
- Partials      150      210      +60

Flag	Coverage Δ
cypress	`?`
javascript	`60.72% <ø> (+0.02%)`	⬆️
python	`64.06% <76.00%> (-0.15%)`	⬇️

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

Impacted Files	Coverage Δ
superset/examples/birth_names.py	`73.19% <76.00%> (-25.65%)`	⬇️
superset-frontend/src/explore/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/dashboard/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/chart/ChartContainer.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/explore/reducers/index.js	`0.00% <0.00%> (-100.00%)`	⬇️
...et-frontend/src/dashboard/containers/Dashboard.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
...et-frontend/src/filters/components/Select/types.ts	`0.00% <0.00%> (-100.00%)`	⬇️
...t-frontend/src/explore/reducers/getInitialState.js	`0.00% <0.00%> (-100.00%)`	⬇️
...-frontend/src/visualizations/presets/MainPreset.js	`0.00% <0.00%> (-100.00%)`	⬇️
...tend/src/dashboard/containers/DashboardBuilder.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
... and 189 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 64b4977...09cfb37. Read the comment docs.

* add birth names fixture * fix birth names related tests * fix test_import_v0_dataset_cli_export columns order * fix celery tests drop table * fix mysql datetime type * fix mysql typo in charts/api_tests * refactor * add licence * fix use fixture for presto * fix presto, hive query * fix flaky metadata * fix mysql bigint type * fix run query * fix hive datatype in metadata * fix slice owner for cypress * refactor num_boys num_girls * fix is_dttm column * debug logging * fix query offset * fix presto ds type in metadata * fix presto ds type * clean up debug logging

pull-request-size bot added the size/XL label Dec 30, 2020

junlincc added the assigned:polidea label Jan 1, 2021

kstrz force-pushed the testing/birth-names branch from d7e0984 to 5c56fb8 Compare January 4, 2021 12:55

willbarrett reviewed Jan 4, 2021

View reviewed changes

dpgaspar reviewed Jan 5, 2021

View reviewed changes

nytai closed this Jan 6, 2021

nytai reopened this Jan 6, 2021

dpgaspar reviewed Jan 6, 2021

View reviewed changes

kstrz force-pushed the testing/birth-names branch from 809b36b to 7ed833b Compare January 7, 2021 06:55

kstrz added 16 commits January 9, 2021 11:20

add birth names fixture

7f49263

fix birth names related tests

20d4891

fix test_import_v0_dataset_cli_export columns order

4db6526

fix celery tests drop table

a7a2eb3

fix mysql datetime type

f6cf38a

fix mysql typo in charts/api_tests

777a976

refactor

7b8014e

add licence

565d13b

fix use fixture for presto

d1db499

fix presto, hive query

0e0cd70

fix flaky metadata

fe8f847

fix mysql bigint type

6a8fa55

fix run query

50f66e6

fix hive datatype in metadata

e56eb4b

fix slice owner for cypress

28b8839

refactor num_boys num_girls

29ec8f9

kstrz force-pushed the testing/birth-names branch from 39e188c to 29ec8f9 Compare January 9, 2021 11:23

kstrz added 2 commits January 9, 2021 20:34

fix is_dttm column

cd81908

debug logging

fb7653f

kstrz force-pushed the testing/birth-names branch from 7e905de to fb7653f Compare January 9, 2021 20:31

kstrz added 3 commits January 9, 2021 21:53

fix query offset

5f2d1b3

fix presto ds type in metadata

ed578f8

fix presto ds type

35644d4

kstrz closed this Jan 11, 2021

kstrz reopened this Jan 11, 2021

clean up debug logging

09cfb37

willbarrett merged commit 2ab490a into apache:master Jan 11, 2021

villebro added the v1.0.1 label Jan 25, 2021

mistercrunch added 🍒 1.0.1 🏷️ 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

test: birth names #12226

test: birth names #12226

kstrz commented Dec 30, 2020

kstrz commented Dec 30, 2020

willbarrett left a comment

willbarrett Jan 4, 2021

kstrz Jan 5, 2021

dpgaspar left a comment

dpgaspar Jan 5, 2021

kstrz Jan 5, 2021

dpgaspar Jan 5, 2021

kstrz Jan 5, 2021

dpgaspar Jan 5, 2021

dpgaspar Jan 5, 2021

kstrz Jan 5, 2021

dpgaspar Jan 5, 2021

kstrz Jan 5, 2021

dpgaspar Jan 5, 2021

kstrz Jan 5, 2021 •

edited

Loading

dpgaspar Jan 6, 2021

dpgaspar Jan 5, 2021

kstrz Jan 5, 2021

dpgaspar Jan 5, 2021

dpgaspar left a comment

codecov-io commented Jan 7, 2021 •

edited

Loading

		create_dashboard(slices)


		def _set_table_metadata(obj: "BaseDatasource", database: "Database") -> None:

test: birth names #12226

test: birth names #12226

Conversation

kstrz commented Dec 30, 2020

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

kstrz commented Dec 30, 2020

willbarrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kstrz Jan 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 7, 2021 • edited Loading

Codecov Report

kstrz Jan 5, 2021 •

edited

Loading

codecov-io commented Jan 7, 2021 •

edited

Loading