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(TestChartApi): move chart data api tests into TestChartDataApi #17407

Merged
merged 4 commits into from
Nov 14, 2021

Conversation

ofekisr
Copy link
Contributor

@ofekisr ofekisr commented Nov 11, 2021

Background

When we have worked on #16991 we wanted to test the new functionalities in concrete and accurate unittest.
All chartData flows and its components are too couple to superset so it is impossible to create unittests.
The flows are not testable and so many components do not meet the very important principle SRP and the code became so dirty

So I've started to refactor it (#17344 ) but many changes were added and it was hard to review so I decided to split those changes into small PRs so will be easier to follow

This is the fourth PR in a sequence of PRs to meet these
The Next PR is #17425

PR description

In #17400, ChartData endpoints were moved to a new view, so the tests should move too.

What else?

  1. Improve tests setup - it doesn't sense each test will do the same setup instead in one place,
  2. improve get_query_contexe setup - it doesn't sense that each test will force call the database over and over. so now it will call the DB once and create a query context template. Then each test will copy the query context template.
  3. Advance another one step further into using pytest without unittest coupling
  4. improve test cases names so it will be clear what the test is done and expected.

Test plans

should we test the tests? :)

Previous PRs

  1. refactor(ChartData): move ChartDataResult enums to common #17399
  2. refactor(ChartData): move chart_data_apis from ChartRestApi ChartDataRestApi #17400
  3. refactor(ChartDataCommand): separate loading query_context form cache into different module #17405

@ofekisr ofekisr changed the title Refactor(TestChartApi): move chart data api tests into TestChartDataApi refactor(TestChartApi): move chart data api tests into TestChartDataApi Nov 11, 2021
@codecov
Copy link

codecov bot commented Nov 11, 2021

Codecov Report

Merging #17407 (cdbcbf3) into master (ad8a7c4) will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17407      +/-   ##
==========================================
+ Coverage   76.97%   77.04%   +0.07%     
==========================================
  Files        1041     1041              
  Lines       56069    56063       -6     
  Branches     7742     7738       -4     
==========================================
+ Hits        43157    43194      +37     
+ Misses      12654    12611      -43     
  Partials      258      258              
Flag Coverage Δ
hive 81.52% <ø> (ø)
mysql 81.95% <ø> (ø)
postgres 81.95% <ø> (ø)
presto 81.82% <ø> (?)
python 82.45% <ø> (+0.14%) ⬆️
sqlite 81.63% <ø> (ø)

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

Impacted Files Coverage Δ
...frontend/src/dashboard/components/Header/index.jsx 68.85% <0.00%> (ø)
...nd/src/explore/components/DataTablesPane/index.tsx 85.71% <0.00%> (+0.12%) ⬆️
superset/models/core.py 90.00% <0.00%> (+0.73%) ⬆️
superset/connectors/sqla/models.py 88.22% <0.00%> (+1.38%) ⬆️
superset/db_engine_specs/presto.py 89.95% <0.00%> (+5.64%) ⬆️

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 ad8a7c4...cdbcbf3. Read the comment docs.

@junlincc junlincc requested a review from nytai November 11, 2021 23:16
@junlincc junlincc added the risk:refactor High risk as it involves large refactoring work label Nov 11, 2021
@junlincc
Copy link
Member

Thanks for the contribution! @ofekisr Looping domain experts in to review your PR. since it's an XXL one with high risk, we may take awhile to get through the review. Please stay tuned.

cc @craig-rueda @betodealmeida @villebro

@ofekisr ofekisr force-pushed the refactor/chart_data_flows_4 branch from 41d47ab to 1358e71 Compare November 12, 2021 12:53
@ofekisr
Copy link
Contributor Author

ofekisr commented Nov 12, 2021

Thanks for the contribution! @ofekisr Looping domain experts in to review your PR. since it's an XXL one with high risk, we may take awhile to get through the review. Please stay tuned.

cc @craig-rueda @betodealmeida @villebro

I sync the branch from master so now it contains only 3 files

@junlincc
Copy link
Member

junlincc commented Nov 12, 2021

@ofekisr could you list the product area that will be affected by your PR, and do a simple audit what's the test coverage look like in those area?

@amitmiran137

@amitmiran137
Copy link
Member

Product area is not affected bc no code changes to the product only to tests
The focus is separate chart/data tests from the rest of the chart domain tests and to properlu name those existing tests with meaningful names

@ofekisr
Copy link
Contributor Author

ofekisr commented Nov 14, 2021

mapping from the old test name (chartApiTest) to the new name in ChartDataApiTest

test_chart_data_simple --> test_with_valid_qc__data_is_returned
test_chart_data_get_no_query_context --> test_get_data_when_query_context_is_null
test_chart_data_get === test_chart_data_get
test_chart_data_applied_time_extras === test_chart_data_applied_time_extras
test_chart_data_limit_offset --> test_with_row_limit_and_offset__row_limit_and_offset_were_applied
test_chart_data_default_row_limit --> test_without_row_limit__row_count_as_default_row_limit
test_chart_data_sql_max_row_limit --> test_with_row_limit_bigger_then_sql_max_row__rowcount_as_sql_max_row
test_chart_data_sample_default_limit --> test_as_samples_without_row_limit__row_count_as_default_samples_row_limit
test_chart_data_sample_custom_limit --> test_with_row_limit_as_samples__rowcount_as_row_limit
test_chart_data_sql_max_row_sample_limit -->
test_as_samples_with_row_limit_bigger_then_sql_max_row__rowcount_as_sql_max_row
test_chart_data_incorrect_result_type --> test_with_incorrect_result_type__400
test_chart_data_incorrect_result_format --> test_with_incorrect_result_format__400
test_chart_data_invalid_form_data --> test_with_invalid_payload__400
test_chart_data_query_result_type --> test_with_query_result_type__200
test_chart_data_csv_result_format --> test_with_csv_result_format
test_chart_data_csv_result_format_permission_denined -->
test_with_csv_result_format_when_actor_not_permitted_for_csv__403
test_chart_data_mixed_case_filter_op --> test_with_in_op_filter__data_is_returned
test_chart_data_dttm_filter === test_chart_data_dttm_filter
test_chart_data_prophet === test_chart_data_prophet
test_chart_data_query_missing_filter --> test_with_query_result_type_and_non_existent_filter__filter_omitted
test_chart_data_no_data --> test_with_filter_suppose_to_return_empty_data__no_data_returned
test_chart_data_incorrect_request --> test_with_invalid_where_parameter__400
test_chart_data_with_invalid_datasource --> test_with_invalid_datasource__400
test_chart_data_with_invalid_enum_value --> test_with_invalid_time_range_endpoints_enum_value__400
test_query_exec_not_allowed --> test_with_not_permitted_actor__401
test_chart_data_jinja_filter_request -->
test_when_where_parameter_is_template_and_query_result_type__query_is_templated
test_chart_data_async === test_chart_data_async
test_chart_data_async_cached_sync_response === test_chart_data_async_cached_sync_response
test_chart_data_async_results_type === test_chart_data_async_results_type
test_chart_data_async_invalid_token === test_chart_data_async_invalid_token
test_chart_data_cache === test_chart_data_cache
test_chart_data_cache_run_failed === test_chart_data_cache_run_failed
test_chart_data_cache_no_login === test_chart_data_cache_no_login
test_chart_data_cache_key_error === test_chart_data_cache_key_error
test_chart_data_annotations --> test_with_annotations_layers__annotations_data_returned
test_chart_data_rowcount === test_chart_data_rowcount
test_chart_data_timegrains -->test_with_timegrains_and_columns_result_types
test_chart_data_series_limit --> test_with_series_limit

@amitmiran137 amitmiran137 removed the risk:refactor High risk as it involves large refactoring work label Nov 14, 2021
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.

Reviewed that entire 2 suites of tests move and renaming and all test remain as the original.
Some boiler plate code was moved to setup
LGTM!

@amitmiran137 amitmiran137 merged commit d8851c9 into apache:master Nov 14, 2021
@villebro
Copy link
Member

This was a nice improvement and provides for a much better chart API development experience!

AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
…pi (#17407)

* refactor charts api tests

* move new added test

* refactor charts api tests
@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/XXL 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants