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: QueryContext and QueryObject by decoupling and making it testable #17344

Closed
wants to merge 5 commits into from

Conversation

ofekisr
Copy link
Contributor

@ofekisr ofekisr commented Nov 4, 2021

A preparation task for #16991.

decoupling QueryContext and QueryObject classes from superset so they can be used in unit tests and future usages.

What have done:

  1. separating the concerns: holding query parameters, objects creations (using factories), and query processing ( data retrieving) into separate modules and Classes.
  2. move utils method to utils module
  3. code cleanup

It relies on the current test cases since no logic was added

follows up:

  1. query context and query context processor naming refactoring
  2. keep refactoring query context processor: add factory, create processor for specific query context and not process them as procedure paradigm and remove the "ping-pong" calls with query_actions.
  3. add caching parameters to query context
  4. remove duplicates code

@ofekisr ofekisr changed the title refactor QueryContext and QueryObject so they will be testable and le… refactor: QueryContext, and QueryObject so they will be testable and le… Nov 4, 2021
@amitmiran137 amitmiran137 changed the title refactor: QueryContext, and QueryObject so they will be testable and le… refactor: QueryContext and QueryObject by decoupling and making it more testable Nov 4, 2021
@amitmiran137 amitmiran137 changed the title refactor: QueryContext and QueryObject by decoupling and making it more testable refactor: QueryContext and QueryObject by decoupling and making it testable Nov 4, 2021
@ofekisr ofekisr marked this pull request as ready for review November 8, 2021 12:26
@codecov
Copy link

codecov bot commented Nov 8, 2021

Codecov Report

Merging #17344 (4fedf18) into master (ee87b01) will decrease coverage by 0.18%.
The diff coverage is 90.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17344      +/-   ##
==========================================
- Coverage   77.14%   76.95%   -0.19%     
==========================================
  Files        1036     1040       +4     
  Lines       55759    55892     +133     
  Branches     7628     7628              
==========================================
- Hits        43013    43012       -1     
- Misses      12490    12624     +134     
  Partials      256      256              
Flag Coverage Δ
hive ?
mysql 81.94% <90.26%> (+0.03%) ⬆️
postgres 81.95% <90.26%> (+0.03%) ⬆️
presto ?
python 82.04% <90.26%> (-0.38%) ⬇️
sqlite 81.63% <90.06%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
superset/viz.py 57.89% <14.28%> (-0.11%) ⬇️
superset/views/api.py 70.68% <42.85%> (-0.74%) ⬇️
superset/common/query_context.py 82.05% <58.33%> (-8.86%) ⬇️
superset/tasks/async_queries.py 95.77% <87.50%> (-1.20%) ⬇️
superset/charts/query_context_cache_loader.py 90.00% <90.00%> (ø)
superset/common/query_object_factory.py 90.62% <90.62%> (ø)
superset/common/query_context_processor.py 91.19% <91.19%> (ø)
superset/charts/api.py 86.58% <91.35%> (+0.75%) ⬆️
superset/common/query_factory.py 94.59% <94.59%> (ø)
superset/common/query_object.py 92.85% <97.50%> (ø)
... and 17 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 ee87b01...4fedf18. Read the comment docs.

@betodealmeida
Copy link
Member

It relies on the current test cases since no logic was added

Note that this is super risky, because we don't have 100% unit test coverage.

@amitmiran137
Copy link
Member

It relies on the current test cases since no logic was added

Note that this is super risky because we don't have 100% unit test coverage.

I agree , but on the other hand if we want to move towards better coverage we need to start refactoring and do separation of concerns

@suddjian
Copy link
Member

suddjian commented Nov 10, 2021

I think if it's possible to split this change into smaller PRs, that will make it much more reviewable. Also, as the purpose of this PR is to make the code testable, I think it would be appropriate and necessary to add tests as part of the refactor.

And if there are any additional tests that cover parts of this, would you mind linking to them? As a reviewer I find tests very helpful for understanding the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants