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

[test] Fix test data remove slice_name #7898

Merged

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Jul 19, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

When user save dashboard, we allow user do not user original slice_name saved in Slice table, and have a copy of slice name which is only used by a dashboard. We also allow user to set this dashboard's slice name be empty.

in front-end code i think we copy slice_name into layout data and send with save_dash API. But in unit tests we didn't set slicename attribute in the mock layout data, and cause after /save_dash/ api call in unit test, sample slices created for unit tests lost their slice_name. So that some unit tests that rely on slice_name uniqueness became flacky.

Solution
I fixed the logic in save_dash. It will only reset slice_name when has explicit attribute in layout.

TEST PLAN

CI

REVIEWERS

@john-bodley @mistercrunch

@codecov-io
Copy link

codecov-io commented Jul 19, 2019

Codecov Report

Merging #7898 into master will increase coverage by <.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7898      +/-   ##
==========================================
+ Coverage   65.76%   65.76%   +<.01%     
==========================================
  Files         461      461              
  Lines       22188    22190       +2     
  Branches     2425     2425              
==========================================
+ Hits        14592    14594       +2     
  Misses       7475     7475              
  Partials      121      121
Impacted Files Coverage Δ
superset/views/core.py 71.66% <33.33%> (-0.37%) ⬇️
superset/utils/core.py 88.08% <0%> (+0.16%) ⬆️
superset/db_engine_specs/sqlite.py 66.66% <0%> (+20.83%) ⬆️

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 ccedbea...fea173b. Read the comment docs.

@graceguo-supercat graceguo-supercat changed the title [text] Fix test data remove slice_name [test] Fix test data remove slice_name Jul 19, 2019
@@ -1974,7 +1974,8 @@ def _set_dash_metadata(dashboard, data):
):
slice_id = value.get("meta").get("chartId")
slice_ids.append(slice_id)
slice_id_to_name[slice_id] = value.get("meta").get("sliceName")
if "sliceName" in value.get("meta"):
slice_id_to_name[slice_id] = value.get("meta").get("sliceName")
Copy link
Member

Choose a reason for hiding this comment

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

Because on line #1970 and #1977 you know that both the meta and sliceName key exists I think it's clearer if you simply do,

slice_id_to_name[slice_id] = value["meta"]["sliceName"]

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

@@ -1974,7 +1974,8 @@ def _set_dash_metadata(dashboard, data):
):
slice_id = value.get("meta").get("chartId")
slice_ids.append(slice_id)
slice_id_to_name[slice_id] = value.get("meta").get("sliceName")
if "sliceName" in value.get("meta"):
Copy link
Member

Choose a reason for hiding this comment

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

Somewhat unrelated to your change but I sense we can cleanup some of this logic (which could be error prone as get() returns None if the key doesn't exist),

if (
     isinstance(value, dict)
     and "meta" in value 
     and "chartId" in value["meta"]
):
    slice_id = value["meta"]["chartId"]
    slice_ids.append(slice_id)
    if "sliceName" in value["meta"]:
        slice_id_to_name[slice_id] = value["meta"]["sliceName"]    

or

try:
    slice_id = value["meta"]["chartId"]
    ...
except KeyError:
    pass

Note I'm uncertain whether sliceId or sliceName could be None and whether additional checks are required.

Copy link
Author

@graceguo-supercat graceguo-supercat Jul 19, 2019

Choose a reason for hiding this comment

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

meta, sliceId, sliceName could be None.
user doesn't have to show a slice with name in dashboard, but the associated Slice entry in the database still have name column

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

@john-bodley
Copy link
Member

@graceguo-supercat thanks for tracing down the root cause of the issue. I had a couple of small comments but otherwise this LGTM.

@graceguo-supercat graceguo-supercat merged commit f570b45 into apache:master Jul 19, 2019
alex-mark pushed a commit to alex-mark/incubator-superset that referenced this pull request Jul 29, 2019
* [text] Fix test data remove slice_name

* fix review comment
@graceguo-supercat graceguo-supercat deleted the gg-FixUnitTestClearSliceName branch August 8, 2019 20:39
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 28, 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/S 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants