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.

Sign up for GitHub

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

Jump to bottom

test: use runner on different timezone #1537

Merged
merged 7 commits into from
Nov 19, 2022
Merged

test: use runner on different timezone #1537

merged 7 commits into from
Nov 19, 2022

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 added p: 2-high Fix main branch c: test s: accepted Accepted feature / Confirmed bug labels Nov 7, 2022
@Shinigami92 Shinigami92 added this to the v8.0 - Module Re-Shuffling milestone Nov 7, 2022
@Shinigami92 Shinigami92 self-assigned this Nov 7, 2022
@Shinigami92 Shinigami92 requested a review from a team as a code owner November 7, 2022 17:24
@codecov
Copy link

codecov bot commented Nov 7, 2022 •
edited
Loading

Codecov Report

Merging #1537 (aabc281) into next (dfd74f2) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files 
@@           Coverage Diff           @@
##             next    #1537   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files        2214     2214           
  Lines      238815   238815           
  Branches     1029     1029           
=======================================
  Hits       237953   237953           
  Misses        841      841           
  Partials       21       21

@Shinigami92 Shinigami92 requested a review from a team November 7, 2022 17:56
@ST-DDT ST-DDT added s: needs decision Needs team/maintainer decision and removed s: accepted Accepted feature / Confirmed bug labels Nov 7, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Nov 7, 2022

IMO there is something else going on here.
Which timezone do you use for your system?

Is there anyone else that has trouble generating/updating the test snapshots files?

ST-DDT
ST-DDT requested changes Nov 7, 2022
View reviewed changes
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Does not work for me:

- Date: Tue Dec 31 2019 13:03:15 GMT+0000 (Coordinated Universal Time)
+ Date: Tue Dec 31 2019 13:03:15 GMT+0000 (Koordinierte Weltzeit)

@ST-DDT
Copy link
Member

ST-DDT commented Nov 7, 2022 •
edited
Loading

FFR: This PR broke the tests: #1512, but the bug existed even before that, we just don't had a test that touched it.

@Shinigami92
Copy link
Member Author

Oh yeah you are right, previously we didn't had snapshot tests for that
Okay, lets rewrite the implementation in another PR

@Shinigami92 Shinigami92 added s: on hold Blocked by something or frozen to avoid conflicts p: 2-high Fix main branch and removed p: 2-high Fix main branch labels Nov 7, 2022
@Shinigami92 Shinigami92 marked this pull request as draft November 7, 2022 20:19
@ST-DDT ST-DDT mentioned this pull request Nov 7, 2022
Merged
@Shinigami92 Shinigami92 removed the p: 2-high Fix main branch label Nov 8, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Nov 10, 2022

Team decision

We will add a runner instead, that runs in non-UTC and non-EN to highlight these issues in the future.

@Shinigami92 Shinigami92 changed the title test: enforce UTC for tests test: use runner on different timezone Nov 10, 2022
@Shinigami92 Shinigami92 added p: 1-normal Nothing urgent and removed s: needs decision Needs team/maintainer decision s: on hold Blocked by something or frozen to avoid conflicts labels Nov 10, 2022
@Shinigami92 Shinigami92 requested a review from ST-DDT November 10, 2022 20:34
ST-DDT
ST-DDT reviewed Nov 10, 2022
View reviewed changes
@Shinigami92 Shinigami92 marked this pull request as ready for review November 15, 2022 17:43
@Shinigami92 Shinigami92 requested a review from ST-DDT November 15, 2022 17:44
@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug c: infra Changes to our infrastructure or project setup labels Nov 15, 2022
@Shinigami92 Shinigami92 merged commit 73690e4 into next Nov 19, 2022
@Shinigami92 Shinigami92 deleted the test-use-utc-for-tests branch November 19, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT approved these changes

@ejcheng ejcheng ejcheng approved these changes

Assignees

@Shinigami92 Shinigami92

Labels
c: infra Changes to our infrastructure or project setup c: test p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v8.0 - Module Re-Shuffling
Development

Successfully merging this pull request may close these issues.
3 participants
@Shinigami92 @ST-DDT @ejcheng