-
Notifications
You must be signed in to change notification settings - Fork 14k
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
fix: Removetime_range_endpoints from query context object pt 2 #19728
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19728 +/- ##
=======================================
Coverage 66.51% 66.51%
=======================================
Files 1686 1686
Lines 64591 64591
Branches 6636 6636
=======================================
Hits 42961 42961
Misses 19931 19931
Partials 1699 1699
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
queries = query_context.get("queries") | ||
for q in queries: | ||
extras = q.get("extras", {}) | ||
assert "time_range_endpoints" not in extras |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! A test on a migration!
"datasource": "27__table", | ||
"slice_id": 545, | ||
"url_params": {}, | ||
"time_range_endpoints": ["inclusive", "exclusive"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
total nit, but technically this wouldn't be on this payload.
maybe not necessary, but if the original migration doesn't work, maybe we can change it to noop? |
try: | ||
query_context = json.loads(slc.query_context) | ||
except json.decoder.JSONDecodeError: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should return here, as the json parse failed
} | ||
|
||
|
||
def test_upgrade(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for the case when the query_context is non-json?
🏷️ preset:2022.15 |
SUMMARY
original reference this time we actually save the data properly 😪
#19423
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION