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

refactor: examples data loading for tests #17893

Merged
merged 3 commits into from
Jan 11, 2022

Conversation

ofekisr
Copy link
Contributor

@ofekisr ofekisr commented Dec 29, 2021

The way the examples data is generated and loaded into the database as SQL data and as superset objects
is not flexible, configurable and in case you would like deterministic data it does not support it.
In addition, there are many reusing ideas and duplicated codes.

For example, in another PR I'm trying to develop I encountered surprising behavior when I tried to load world bank data.
I thought the way it is loaded and cleaned up be the same as loading the birth_names data but no.

This PR is the first one of achieving this. It abstract the way the raw data is generated.

@amitmiran137 amitmiran137 changed the title Test/refactor data loading refactor: examples data loading for tests Dec 29, 2021
@ofekisr ofekisr marked this pull request as ready for review December 30, 2021 11:27
@codecov
Copy link

codecov bot commented Dec 30, 2021

Codecov Report

Merging #17893 (cfe2c2e) into master (8ebec60) will decrease coverage by 0.17%.
The diff coverage is 74.62%.

❗ Current head cfe2c2e differs from pull request most recent head bc7568a. Consider uploading reports for the commit bc7568a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17893      +/-   ##
==========================================
- Coverage   67.10%   66.92%   -0.18%     
==========================================
  Files        1609     1612       +3     
  Lines       64897    64988      +91     
  Branches     6866     6872       +6     
==========================================
- Hits        43547    43495      -52     
- Misses      19484    19626     +142     
- Partials     1866     1867       +1     
Flag Coverage Δ
hive 53.32% <ø> (-28.50%) ⬇️
mysql ?
postgres ?
presto 53.16% <ø> (-28.96%) ⬇️
python 82.32% <ø> (-0.42%) ⬇️
sqlite 81.94% <ø> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...-chart-controls/src/sections/advancedAnalytics.tsx 33.33% <ø> (ø)
...ntend/packages/superset-ui-core/src/color/index.ts 100.00% <ø> (ø)
...s/legacy-plugin-chart-country-map/src/countries.ts 100.00% <ø> (ø)
...legacy-plugin-chart-partition/src/controlPanel.tsx 25.00% <ø> (ø)
...gins/legacy-plugin-chart-rose/src/controlPanel.tsx 50.00% <ø> (ø)
.../plugins/legacy-preset-chart-nvd3/src/Bar/index.js 66.66% <ø> (ø)
...gins/legacy-preset-chart-nvd3/src/DistBar/index.js 66.66% <ø> (ø)
...ins/legacy-preset-chart-nvd3/src/DualLine/index.js 66.66% <ø> (ø)
...gins/legacy-preset-chart-nvd3/src/NVD3Controls.tsx 95.83% <ø> (ø)
...plugins/legacy-preset-chart-nvd3/src/ReactNVD3.jsx 0.00% <ø> (ø)
... and 99 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 8ebec60...bc7568a. 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.

A few minor comments, but I agree we could do with more consolidation on the example data front, as it has evolved over a long time and is inconsistent. One main question I have is can you elaborate on the need for the generator/factory/impl/singleton design being proposed here? I'm not opposed to it, but it feels like a lot of abstraction for something that may not be required. For example, in the fixture where we do return list(BirthNamesGeneratorFactory.make().generate()), I wonder if we couldn't just do something like return BirthNamesExample.get_data()? Point being, if we don't anticipate we'll need a generator (here we're just putting list` around what's being yielded) or a full blown singleton pattern, why add one?

@ofekisr
Copy link
Contributor Author

ofekisr commented Jan 11, 2022

@villebro @amitmiran137
I added tests for common
I changed to use "..."

regarding the abstractions: this is how I'm coding ... design to interfaces
the one who uses the generator should not aware of how it generates (decouple implementations details)
Why do you fear using abstractions?
the singleton is inside the abstract class so the client is only aware of the API. the concrete details of how the generator is built are encapsulated, but there is a way to change the implementations if required and should be done in separate setup file

Copy link
Member

@amitmiran137 amitmiran137 left a comment

Choose a reason for hiding this comment

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

Lgtm

@amitmiran137 amitmiran137 merged commit 7fc6a2f into apache:master Jan 11, 2022
@amitmiran137 amitmiran137 deleted the test/refactor_data_loading branch January 11, 2022 12:16
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
* refactor: replace the way the birth_names data is generated

* refactor: replace the way the birth_names data is generated

* refactor structure
add tests for common
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
* refactor: replace the way the birth_names data is generated

* refactor: replace the way the birth_names data is generated

* refactor structure
add tests for common
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 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/XL 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants