test: removed unicode_test example from unit tests #11131

kkucharc · 2020-10-01T18:43:25Z

SUMMARY

To improve development of unit/integration tests development, here is suggestion of removing unicode_test project examples loaded in tests setup and replacing it with fixtures that creates dashboard with sample of unicode data.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-commenter · 2020-10-02T15:41:53Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #11131      +/-   ##
==========================================
- Coverage   65.79%   60.75%   -5.04%     
==========================================
  Files         816      391     -425     
  Lines       38422    24483   -13939     
  Branches     3621        0    -3621     
==========================================
- Hits        25280    14875   -10405     
+ Misses      13034     9608    -3426     
+ Partials      108        0     -108

Flag	Coverage Δ
#cypress	`?`
#javascript	`?`
#python	`60.75% <ø> (-0.66%)`	⬇️

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

Impacted Files	Coverage Δ
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/examples/unicode_test_data.py	`22.00% <0.00%> (-78.00%)`	⬇️
superset/db_engine_specs/hive.py	`53.90% <0.00%> (-30.08%)`	⬇️
superset/utils/decorators.py	`50.76% <0.00%> (-4.24%)`	⬇️
superset/examples/helpers.py	`95.00% <0.00%> (-2.50%)`	⬇️
superset/models/sql_lab.py	`90.69% <0.00%> (-0.65%)`	⬇️
superset/db_engine_specs/presto.py	`81.42% <0.00%> (-0.50%)`	⬇️
superset/utils/pandas_postprocessing.py	`76.47% <0.00%> (-0.50%)`	⬇️
superset/views/base_api.py	`97.90% <0.00%> (-0.36%)`	⬇️
superset/models/core.py	`88.12% <0.00%> (-0.28%)`	⬇️
... and 470 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 d056e3d...55da74f. Read the comment docs.

willbarrett

Kasha, I think this is a solid approach. Thank you for taking this on, and please continue!

willbarrett · 2020-10-02T16:11:34Z

@villebro @dpgaspar this is the first PR of a refactor to move away from the example data and to Pytest Fixtures. Please help validate the approach before we cross-apply this type of change to the rest of the suite.

tests/dashboard_utils.py

dpgaspar · 2020-10-02T16:20:42Z

tests/dashboard_utils.py

+    return obj
+
+
+def create_slice(title, viz_type, tbl, slices_dict: Dict[str, str]):


nit: missing return type

Fixed, thanks 👍

tests/dashboard_utils.py

dpgaspar · 2020-10-02T16:23:49Z

tests/databases/api_tests.py

+
+            db.session.commit()
+            position = "{}"
+            create_dashboard("unicode-test", "Unicode Test", position, None)


What's being done on other pytest fixtures is to yield create_dashboard("unicode-test", "Unicode Test", position, None) and then clean everything up (on this case delete the dashboard and then delete the table for dashboard)

Thanks for the hint. I changed it this way. Thanks to that I also removed deleting dashboard from conftest.py

tests/security_tests.py

tests/dashboard_utils.py

villebro · 2020-10-04T08:36:54Z

tests/security_tests.py

+    @pytest.fixture()
+    def load_unicode_dashboard(self):


Should we put this in fixtures.py so we don't have to repeat this logic wherever we use the unicode dashboard?

Sounds good. I just wanted to clarify if you mean moving to fixtures/ directory? My only concern is about some fixtures that create slices for dashboards (or some other setup). I wondered if wouldn't be better to have just some common parts to combine those dashboard creation parts into particular fixtures (as there is this new file dashboard_utils. Or maybe having basic fixture which will be extended or decorated in more complex cases? WDYT @villebro ?

@kkucharc you are right, I meant the fixtures/ directory! Perhaps we could create something like fixtures/unicode.py that would for now just include def load_unicode_dashboard(). Down the road I actually think it would be great to try to decouple datasets, charts and dashboards from each other, as may backend tests don't necessarily need the full suite of charts and dashboards, but are ok with just the dataset. Having said that, let's take gradual steps here, so as to not choke on trying to make this perfect on first iteration.

That makes sens. I refactored this part. To be honest, I'm proud of this :) - I hope this will be good step on first iteration of this refactor. WDYT?

Yeah, I really like this refactor! Hi-5 @kkucharc ! 💯

tests/strategy_tests.py

villebro

Second pass, mostly nits. I'm really loving this, super nice! 🚀

villebro · 2020-10-05T18:06:37Z

tests/dashboard_utils.py

+
+
+def create_table_for_dashboard(
+    df: DataFrame, tbl_name: str, database: Database, schema: Dict[str, Any]


nit: I personally find table_name more pythonic.

Suggested change

df: DataFrame, tbl_name: str, database: Database, schema: Dict[str, Any]

df: DataFrame, table_name: str, database: Database, schema: Dict[str, Any]

I am also for more descriptive names instead of shortcuts. I changed it in both of my files.

tests/dashboard_utils.py

tests/fixtures/unicode_dashboard.py

dpgaspar

I like the pattern of having idempotent tests, where the state of the DB is kept between tests.
This is looking good!

What do you think about changing the file location for tests/fixtures/unicode_dashboard.py to tests/dashboards/fixtures.py or tests/dashboards/fixtures/unicode.py for example?

dpgaspar · 2020-10-06T12:15:31Z

tests/fixtures/unicode_dashboard.py

+    with app.app_context():
+        yield _create_unicode_dashboard(df, table_name, "Unicode Cloud", None)
+
+        _cleanup()


the _cleanup() is not actually cleaning up. The dashboard is still there. My idea is to make all test idempotent so we can run them multiple times and would help to clean weird behaviours (for example changing a test run order would make the test fail). So I think we should delete the dashboard also

Sure, you are right. I also added removing slice because apparently it doesn't cleanup after deleting particular dashboard record.

kkucharc · 2020-10-06T12:51:51Z

@dpgaspar Thanks for review!
About moving file, I am not sure if putting this file in tests/dashboards/ would be good idea, since schedules_test.py and security_tests.py which also use it are in tests/ dir. Also actually api_tests.py which uses this fixture is in databases/ - not in dashboards/. But it still creates dashboards for those tests, so I don't have really strong opinion about it.

villebro · 2020-10-07T06:07:32Z

@kkucharc do you prefer getting review comments now or wait until CI is passing?

…veral methods. Added param types and return types.

kkucharc · 2020-10-07T13:07:07Z

I guess it was worth waiting for CI, because apparently cleanup had impact on charts tests. I also rebased because of conflicts. @villebro I think it's ready now :)

villebro

One small comment, but I consider this ready to go. LGTM and great work @kkucharc!

tests/dashboard_utils.py

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

dpgaspar

LGTM!

dpgaspar · 2020-10-07T13:46:21Z

tests/fixtures/unicode_dashboard.py

+    engine.execute("DROP TABLE IF EXISTS unicode_test")
+    db.session.delete(dash)
+    if slice_name:
+        slice = db.session.query(Slice).filter_by(slice_name=slice_name).first()


nit: slice = db.session.query(Slice).filter_by(slice_name=slice_name).one_or_none()

codecov-io · 2020-10-07T13:57:57Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a071393). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #11131   +/-   ##
=========================================
  Coverage          ?   55.99%           
=========================================
  Files             ?      403           
  Lines             ?    13362           
  Branches          ?     3429           
=========================================
  Hits              ?     7482           
  Misses            ?     5696           
  Partials          ?      184

Flag	Coverage Δ
#cypress	`55.99% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out 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 a071393...7c78f7a. Read the comment docs.

tests/dashboard_utils.py

…rst element with one_or_none

* Removed depemdency to unicode example in tests config. * Added common methods for creating dashboards for tests. * Added fixtures to all tests which were using unicode example. * Added cleanup for unicode_test table * Removed unnecessary fixture parts of unicode dashboard tests * Parametrized creating slice for tests * Moved fixtures for unicode test to separate file and refactored to several methods. Added param types and return types. * Cleandup after fix * Changed variable names to more readable * Added cleanup for dashboards and slices * Applied unicode fixture to charts api tests * Update schema variable to dtype in dashboard utils Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> * Changed variable schema to dtype in dashboards. Replaced accessing first element with one_or_none Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

superset-github-bot bot added the polidea label Oct 1, 2020

pull-request-size bot added the size/L label Oct 1, 2020

kkucharc changed the title ~~[WIP] fix: removed unicode_test example from unit tests~~ test: removed unicode_test example from unit tests [WIP] Oct 2, 2020

kkucharc added the test label Oct 2, 2020

kkucharc changed the title ~~test: removed unicode_test example from unit tests [WIP]~~ test: removed unicode_test example from unit tests Oct 2, 2020

kkucharc requested a review from willbarrett October 2, 2020 13:49

willbarrett approved these changes Oct 2, 2020

View reviewed changes

willbarrett requested review from villebro and dpgaspar October 2, 2020 16:10

dpgaspar requested changes Oct 2, 2020

View reviewed changes

villebro requested changes Oct 4, 2020

View reviewed changes

villebro reviewed Oct 5, 2020

View reviewed changes

dpgaspar requested changes Oct 6, 2020

View reviewed changes

kkucharc requested a review from villebro October 6, 2020 17:29

kkucharc added 11 commits October 7, 2020 14:10

Removed depemdency to unicode example in tests config.

bbd1239

Added common methods for creating dashboards for tests.

37ecef0

Added fixtures to all tests which were using unicode example.

173ea9a

Added cleanup for unicode_test table

5ac088f

Removed unnecessary fixture parts of unicode dashboard tests

8002941

Parametrized creating slice for tests

773898b

Moved fixtures for unicode test to separate file and refactored to se…

21e5fe5

…veral methods. Added param types and return types.

Cleandup after fix

2f9e21a

Changed variable names to more readable

c04ce35

Added cleanup for dashboards and slices

7c07907

Applied unicode fixture to charts api tests

8f5ec97

kkucharc force-pushed the testing/unicode_test branch from da61088 to 8f5ec97 Compare October 7, 2020 12:25

kkucharc requested a review from dpgaspar October 7, 2020 13:07

villebro approved these changes Oct 7, 2020

View reviewed changes

tests/dashboard_utils.py Outdated Show resolved Hide resolved

Update schema variable to dtype in dashboard utils

c4537ff

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

dpgaspar approved these changes Oct 7, 2020

View reviewed changes

villebro reviewed Oct 7, 2020

View reviewed changes

tests/dashboard_utils.py Outdated Show resolved Hide resolved

Changed variable schema to dtype in dashboards. Replaced accessing fi…

7c78f7a

…rst element with one_or_none

villebro merged commit 32e174d into apache:master Oct 7, 2020

laxmanraparthi mentioned this pull request Jun 28, 2023

[Snyk] Fix for 13 vulnerabilities ShareChat/superset#15

Open

This was referenced Nov 26, 2023

[Snyk] Fix for 10 vulnerabilities ShareChat/superset#56

Open

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 ShareChat/superset#57

Open

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 ShareChat/superset#61

Open

m-ajay mentioned this pull request Nov 28, 2023

[Snyk] Fix for 15 vulnerabilities m-ajay/superset#200

Open

This was referenced Dec 14, 2023

[Snyk] Fix for 15 vulnerabilities matrix-compute/superset#13

Open

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 matrix-compute/superset#21

Open

This was referenced Dec 20, 2023

[Snyk] Fix for 13 vulnerabilities ShareChat/superset#97

Open

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 ShareChat/superset#100

Open

matrix-compute mentioned this pull request Dec 26, 2023

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 matrix-compute/superset#28

Open

laxmanraparthi mentioned this pull request Jan 2, 2024

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 ShareChat/superset#107

Open

matrix-compute mentioned this pull request Feb 16, 2024

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 matrix-compute/superset#52

Open

matrix-compute mentioned this pull request Mar 11, 2024

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 matrix-compute/superset#68

Merged

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

laxmanraparthi mentioned this pull request Mar 15, 2024

[Snyk] Fix for 22 vulnerabilities ShareChat/superset#156

Open

matrix-compute mentioned this pull request Mar 23, 2024

[Snyk] Fix for 14 vulnerabilities matrix-compute/superset#78

Open

This was referenced May 14, 2024

[Snyk] Fix for 3 vulnerabilities matrix-compute/superset#133

Open

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 matrix-compute/superset#136

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: removed unicode_test example from unit tests #11131

test: removed unicode_test example from unit tests #11131

kkucharc commented Oct 1, 2020 •

edited

Loading

codecov-commenter commented Oct 2, 2020 •

edited

Loading

willbarrett left a comment

willbarrett commented Oct 2, 2020

dpgaspar Oct 2, 2020

kkucharc Oct 5, 2020

dpgaspar Oct 2, 2020

kkucharc Oct 5, 2020

villebro Oct 4, 2020

kkucharc Oct 5, 2020

villebro Oct 5, 2020

kkucharc Oct 5, 2020

villebro Oct 5, 2020

villebro left a comment

villebro Oct 5, 2020

kkucharc Oct 6, 2020

dpgaspar left a comment •

edited

Loading

dpgaspar Oct 6, 2020

kkucharc Oct 6, 2020

kkucharc commented Oct 6, 2020

villebro commented Oct 7, 2020

kkucharc commented Oct 7, 2020

villebro left a comment

dpgaspar left a comment

dpgaspar Oct 7, 2020 •

edited

Loading

codecov-io commented Oct 7, 2020 •

edited

Loading

		return obj


		def create_slice(title, viz_type, tbl, slices_dict: Dict[str, str]):



		def create_table_for_dashboard(
		df: DataFrame, tbl_name: str, database: Database, schema: Dict[str, Any]

	df: DataFrame, tbl_name: str, database: Database, schema: Dict[str, Any]
	df: DataFrame, table_name: str, database: Database, schema: Dict[str, Any]

test: removed unicode_test example from unit tests #11131

test: removed unicode_test example from unit tests #11131

Conversation

kkucharc commented Oct 1, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-commenter commented Oct 2, 2020 • edited Loading

Codecov Report

willbarrett left a comment

Choose a reason for hiding this comment

willbarrett commented Oct 2, 2020

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

villebro 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 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkucharc commented Oct 6, 2020

villebro commented Oct 7, 2020

kkucharc commented Oct 7, 2020

villebro left a comment

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Oct 7, 2020 • edited Loading

Codecov Report

kkucharc commented Oct 1, 2020 •

edited

Loading

codecov-commenter commented Oct 2, 2020 •

edited

Loading

dpgaspar left a comment •

edited

Loading

dpgaspar Oct 7, 2020 •

edited

Loading

codecov-io commented Oct 7, 2020 •

edited

Loading