-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(custom-tz): added the ability to set UTC as the timezone when making custom time range queries #18011
Conversation
558dcbd
to
845c4a2
Compare
const expected = { | ||
lower: setTimeToUTC(lower), | ||
upper: setTimeToUTC(upper), | ||
type: 'custom', |
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.
Rather than hardcoding the values as I was doing, I set the time more deterministically using the setTimeToUTC function.
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.
i'm not sure this test is actually testing much at this point. when you strip everything away it basically says
assertEquals(setTimeToUTC(lower), setTimeToUTC(lower))
since this line is the code under test.
don't think we should remove it, but i do think you should spend some time thinking about ways you can get more confidence from this test.
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.
I totally agree. The only other implementation of this that I could think of would be to calculate the UTCOffset locally and set a value dynamically through a switch statement with all potential values
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.
LGTM. comment about the tests being tautological, but dont think you should undo all the test work right now.
const expected = { | ||
lower: setTimeToUTC(lower), | ||
upper: setTimeToUTC(upper), | ||
type: 'custom', |
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.
i'm not sure this test is actually testing much at this point. when you strip everything away it basically says
assertEquals(setTimeToUTC(lower), setTimeToUTC(lower))
since this line is the code under test.
don't think we should remove it, but i do think you should spend some time thinking about ways you can get more confidence from this test.
…ing a query with a custom timeRange Need to integrate this functionality for the queryBuilder
…ueryBuilder to correct set keys and values
…ng custom timeRanges to UTC by creating a selector to handle that logic wrote tests to test the new selector
…imeZone in AppState mock
Update the .circleci config file to set the timezone
…dated mockState to see whether the test failures happen differently
…xpected result so that it's more deterministic
…e consistent throughout
… from setTimetoUTC
a8a1b09
to
febc6bf
Compare
Closes #17877
Purpose
The primary purpose of this PR is to ensure that whenever a user sets UTC as their timeZone and is querying a custom timeRange, their query will query data that was set at that set time in UTC (not the adjusted locale -> UTC time).
Problem
The UI for the timeZone (local or UTC) was often confusing and seemed to indicate functionality beyond displaying graphs in UTC.
Solution
Created a selector to update the timeRange whenever a query is executed with a custom timeRange and the timeZone set to UTC.