You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/12/30 11:42:59 UTC
[GitHub] [incubator-superset] kstrz opened a new pull request #12226: test: birth names
kstrz opened a new pull request #12226:
URL: https://github.com/apache/incubator-superset/pull/12226
### SUMMARY
Replace birth names example with fixtures for testing.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
### TEST PLAN
All tests should pass.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kstrz commented on a change in pull request #12226: test: birth names
Posted by GitBox <gi...@apache.org>.
kstrz commented on a change in pull request #12226:
URL: https://github.com/apache/incubator-superset/pull/12226#discussion_r551908115
##########
File path: 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")
Review comment:
I think it' 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kstrz commented on a change in pull request #12226: test: birth names
Posted by GitBox <gi...@apache.org>.
kstrz commented on a change in pull request #12226:
URL: https://github.com/apache/incubator-superset/pull/12226#discussion_r551944217
##########
File path: superset/examples/birth_names.py
##########
@@ -108,11 +108,24 @@ def load_birth_names(
print(f"Creating table [{tbl_name}] reference")
obj = TBL(table_name=tbl_name)
db.session.add(obj)
- obj.main_dttm_col = "ds"
+
+ _set_table_metadata(obj, database)
+ _add_table_metrics(obj)
+
+ db.session.commit()
+
+ slices, _ = create_slices(obj)
+ create_dashboard(slices)
+
+
+def _set_table_metadata(obj: "BaseDatasource", database: "Database") -> None:
+ obj.main_dttm_col = "ds" # type: ignore
obj.database = database
obj.filter_select_enabled = True
obj.fetch_metadata()
+
+def _add_table_metrics(obj: "BaseDatasource") -> None:
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kstrz commented on a change in pull request #12226: test: birth names
Posted by GitBox <gi...@apache.org>.
kstrz commented on a change in pull request #12226:
URL: https://github.com/apache/incubator-superset/pull/12226#discussion_r551944305
##########
File path: tests/charts/api_tests.py
##########
@@ -435,6 +436,11 @@ def test_create_chart(self):
"""
Chart API: Test create chart
"""
+ dashes = (
+ db.session.query(Dashboard)
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull request #12226: test: birth names
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #12226:
URL: https://github.com/apache/incubator-superset/pull/12226#discussion_r551619626
##########
File path: 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}")
Review comment:
Should this be refactored into a shared teardown method?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kstrz commented on pull request #12226: test: birth names
Posted by GitBox <gi...@apache.org>.
kstrz commented on pull request #12226:
URL: https://github.com/apache/incubator-superset/pull/12226#issuecomment-752633285
Hey! Could you look at it? @willbarrett @dpgaspar
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kstrz commented on a change in pull request #12226: test: birth names
Posted by GitBox <gi...@apache.org>.
kstrz commented on a change in pull request #12226:
URL: https://github.com/apache/incubator-superset/pull/12226#discussion_r551944049
##########
File path: 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}")
Review comment:
done
##########
File path: superset/examples/birth_names.py
##########
@@ -108,11 +108,24 @@ def load_birth_names(
print(f"Creating table [{tbl_name}] reference")
obj = TBL(table_name=tbl_name)
db.session.add(obj)
- obj.main_dttm_col = "ds"
+
+ _set_table_metadata(obj, database)
+ _add_table_metrics(obj)
+
+ db.session.commit()
+
+ slices, _ = create_slices(obj)
+ create_dashboard(slices)
+
+
+def _set_table_metadata(obj: "BaseDatasource", database: "Database") -> None:
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kstrz commented on a change in pull request #12226: test: birth names
Posted by GitBox <gi...@apache.org>.
kstrz commented on a change in pull request #12226:
URL: https://github.com/apache/incubator-superset/pull/12226#discussion_r551944439
##########
File path: tests/dashboards/api_tests.py
##########
@@ -1147,7 +1152,12 @@ def test_export_bundle(self):
"""
Dashboard API: Test dashboard export
"""
- argument = [1, 2]
+ dashes = (
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kstrz commented on a change in pull request #12226: test: birth names
Posted by GitBox <gi...@apache.org>.
kstrz commented on a change in pull request #12226:
URL: https://github.com/apache/incubator-superset/pull/12226#discussion_r551903712
##########
File path: 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")
Review comment:
Do you mean leaving sth like this: `engine.execute("DROP TABLE IF EXISTS birth_names")`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #12226: test: birth names
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #12226:
URL: https://github.com/apache/incubator-superset/pull/12226#discussion_r551805150
##########
File path: superset/examples/birth_names.py
##########
@@ -108,11 +108,24 @@ def load_birth_names(
print(f"Creating table [{tbl_name}] reference")
obj = TBL(table_name=tbl_name)
db.session.add(obj)
- obj.main_dttm_col = "ds"
+
+ _set_table_metadata(obj, database)
+ _add_table_metrics(obj)
+
+ db.session.commit()
+
+ slices, _ = create_slices(obj)
+ create_dashboard(slices)
+
+
+def _set_table_metadata(obj: "BaseDatasource", database: "Database") -> None:
+ obj.main_dttm_col = "ds" # type: ignore
obj.database = database
obj.filter_select_enabled = True
obj.fetch_metadata()
+
+def _add_table_metrics(obj: "BaseDatasource") -> None:
Review comment:
nit: same here
##########
File path: superset/examples/birth_names.py
##########
@@ -108,11 +108,24 @@ def load_birth_names(
print(f"Creating table [{tbl_name}] reference")
obj = TBL(table_name=tbl_name)
db.session.add(obj)
- obj.main_dttm_col = "ds"
+
+ _set_table_metadata(obj, database)
+ _add_table_metrics(obj)
+
+ db.session.commit()
+
+ slices, _ = create_slices(obj)
+ create_dashboard(slices)
+
+
+def _set_table_metadata(obj: "BaseDatasource", database: "Database") -> None:
Review comment:
nit: `obj: "BaseDatasource"` is called `tbl: "BaseDatasource"` on `create_slices` we could name them equally maybe `datasource`?
##########
File path: 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")
Review comment:
Should we use `IF EXISTS` here also?
##########
File path: 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)
Review comment:
nice, this was probably flaky
##########
File path: tests/charts/api_tests.py
##########
@@ -435,6 +436,11 @@ def test_create_chart(self):
"""
Chart API: Test create chart
"""
+ dashes = (
+ db.session.query(Dashboard)
Review comment:
nit: we can just select the id's here `db.session.query(Dashboard.id)`
##########
File path: 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")
Review comment:
If this is replicated on almost every method, can we use the fixture at the module level?
##########
File path: 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
Review comment:
nice!
##########
File path: tests/dashboards/api_tests.py
##########
@@ -1147,7 +1152,12 @@ def test_export_bundle(self):
"""
Dashboard API: Test dashboard export
"""
- argument = [1, 2]
+ dashes = (
Review comment:
nit: would be nice to refactor and make this DRY
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kstrz commented on a change in pull request #12226: test: birth names
Posted by GitBox <gi...@apache.org>.
kstrz commented on a change in pull request #12226:
URL: https://github.com/apache/incubator-superset/pull/12226#discussion_r551908115
##########
File path: 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")
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org