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] makeDateTrunc calculating rolling_month wrong #52579
Conversation
Also fixed the function returning a string without a timezone, when it always returns times in UTC. This is done by adding 'Z' at the end of the date string. Added unit tests as well, to make sure everything works as expected.
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!
// by the unit parameter (e.g. day, week, month, or rolling month [prior 1 month]). | ||
// Note: If unit is 'week', the function will truncate to the preceding Sunday. | ||
// This is because some locales start the week on Sunday, unlike the Postgres default | ||
// (and many parts of the world) which start the week on Monday. |
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.
and many parts of the world
😁😁😁
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 asked Cody to generate that, so shifting blame to it 😅
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.
nono, liked that part, actually :)
@@ -2027,3 +2027,80 @@ func TestEventLogs_AggregatedRepoMetadataStats(t *testing.T) { | |||
}) | |||
} | |||
} | |||
|
|||
func TestMakeDateTruncExpression(t *testing.T) { |
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.
love the tests!
internal/database/event_logs_test.go
Outdated
|
||
require.Equal(t, tc.expected, date) |
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.
we can use assert
here so that we always run all the test cases and accumulate all the errors instead of short-circuiting the test with the first failure.
require.Equal(t, tc.expected, date) | |
assert.Equal(t, tc.expected, date) |
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.
TIL, I was not aware of that behavior.
It was actually running all the tests before for me even with require.Equal. Just that each test case failed on the first failure.
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.
yep, assert
functions effectively call t.Errorf
, require
functions call t.FailNow()
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-5.0 5.0
# Navigate to the new working tree
cd .worktrees/backport-5.0
# Create a new branch
git switch --create backport-52579-to-5.0
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6d2a71eb07b885346b677eee150930abb7e8910f
# Push it to GitHub
git push --set-upstream origin backport-52579-to-5.0
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-5.0 Then, create a pull request where the |
Also fixed the function returning a string without a timezone, when it always returns times in UTC. This is done by explicitly casting to timestamptz by using `TIMEZONE('UTC', ...)` function. Added unit tests as well, to make sure everything works as expected. Unit tests (cherry picked from commit 6d2a71e)
Also fixed the function returning a string without a timezone, when it always returns times in UTC. This is done by explicitly casting to timestamptz by using `TIMEZONE('UTC', ...)` function. Added unit tests as well, to make sure everything works as expected. ## Test plan Unit tests
Also fixed the function returning a string without a timezone, when it always returns times in UTC. This is done by explicitly casting to timestamptz by using
TIMEZONE('UTC', ...)
function.Added unit tests as well, to make sure everything works as expected.
Test plan
Unit tests