Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: disable tests broken by daylight savings#10414

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

spikecurtis
Copy link
Contributor

Timezone tests hard-code the offset, which is now different due to daylight savings

@spikecurtisGraphite App
Copy link
ContributorAuthor

Current dependencies on/for this PR:

This comment was auto-generated byGraphite.

Copy link

@cdr-botcdr-botbot left a comment

Choose a reason for hiding this comment

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

This PR is a hotfix and has been automatically approved.

  • ✅ Base is main
  • ✅ Has hotfix label
  • ✅ Head is from coder/coder
  • ✅ Less than 100 lines

@spikecurtisspikecurtisenabled auto-merge (squash)October 30, 2023 06:39
@spikecurtisspikecurtis merged commitc2e3648 intomainOct 30, 2023
@spikecurtisspikecurtis deleted the 10-30-fix_disable_tests_broken_by_daylight_savings branchOctober 30, 2023 06:44
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsOct 30, 2023
@Emyrk
Copy link
Member

Oof. So either need to find locations without daylight savings or detect it and change the test accordingly?

@spikecurtis
Copy link
ContributorAuthor

Oof. So either need to find locations without daylight savings or detect it and change the test accordingly?

The other thought I had is to make that function also take thetime.Time, which would betime.Now() in production, but could be a known time with a known offset in tests.

Anyway, what real value are we getting by asserting particular US/Ireland location offsets? Could also just replace those commented out test cases with Abu Dhabi and call it a day.

@Emyrk
Copy link
Member

Yea, I started the refactor to pass in a time, I also usestime.Time.IsDST() to adjust the test to allow DST to pass.

In the end though, the refactor to pass in a time kinda makes the function api a little strange since thetime.Time has a.Location(), and you also pass in atime.Location?

In the end this is for insights to make sure the data is relative to the user's local timezone, and the buckets are daily. So I stopped spending time on getting the tests to be "perfect". 🤷

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@cdr-botcdr-bot[bot]cdr-bot[bot] approved these changes

@EmyrkEmyrkAwaiting requested review from Emyrk

Assignees

@spikecurtisspikecurtis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@spikecurtis@Emyrk

[8]ページ先頭

©2009-2025 Movatter.jp