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.

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: datatype.datetime should use static boundaries #343

Merged
merged 2 commits into from
Mar 24, 2022

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Jan 29, 2022

Fixes #334

new Date().setFullYear(...) does not return a consistent value, as the actual time is not fixed an thus depends on the execution time + timezone.

This PR uses UTC based static values instead.

@ST-DDT ST-DDT added the c: bug Something isn't working label Jan 29, 2022
@ST-DDT ST-DDT self-assigned this Jan 29, 2022
@ST-DDT ST-DDT requested a review from a team as a code owner January 29, 2022 12:02
@ST-DDT ST-DDT added this to the v6.1 - First bugfixes milestone Jan 29, 2022
@pkuczynski
Copy link
Member

Nice catch and fix! We use new Date() in date.te too, and I think I have seen some test failing around the midnight where the local time zone was the next day vs utc. Maybe it would be worth to apply it over there too?

pkuczynski
pkuczynski previously approved these changes Jan 29, 2022
@Shinigami92
Copy link
Member

Oh my god (╯°□°)╯︵ ┻━┻

@ST-DDT
Copy link
Member Author

ST-DDT commented Jan 29, 2022

Nice catch and fix! We use new Date() in date.te too, and I think I have seen some test failing around the midnight where the local time zone was the next day vs utc. Maybe it would be worth to apply it over there too?

The date module uses either new Date() with refers to a relative date or uses the dateRef parameter.
So it should be stable. If you catch a failing test, please report it so that we can analyze it and check only the related methods.

@ST-DDT
Copy link
Member Author

ST-DDT commented Jan 29, 2022

This needs to be rebased after #344 is merged.

@ST-DDT
Copy link
Member Author

ST-DDT commented Jan 29, 2022

Rebased

Shinigami92
Shinigami92 previously approved these changes Jan 29, 2022
@ST-DDT ST-DDT requested a review from a team January 29, 2022 20:53
@ST-DDT ST-DDT force-pushed the fix/datatype/datatime-seeding branch from 824c251 to 6a700ea Compare January 30, 2022 21:16
@ST-DDT ST-DDT requested a review from Shinigami92 January 30, 2022 21:16
@ST-DDT ST-DDT force-pushed the fix/datatype/datatime-seeding branch from 6a700ea to 4e8f9a7 Compare February 6, 2022 15:51
@codecov
Copy link

codecov bot commented Feb 6, 2022

Codecov Report

Merging #343 (ee342c8) into main (301a6d2) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
- Coverage   99.33%   99.33%   -0.01%     
==========================================
  Files        1923     1923              
  Lines      176860   176860              
  Branches      917      915       -2     
==========================================
- Hits       175692   175690       -2     
- Misses       1112     1114       +2     
  Partials       56       56              
Impacted Files Coverage Δ
src/datatype.ts 99.66% <100.00%> (ø)
src/vendor/unique.ts 93.54% <0.00%> (-1.62%) ⬇️

pkuczynski
pkuczynski previously approved these changes Feb 6, 2022
Copy link
Member

@pkuczynski pkuczynski left a comment

Choose a reason for hiding this comment

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

Thats what I mentioned the other day. Cool that you fixed it :)

Shinigami92
Shinigami92 previously approved these changes Mar 10, 2022
@Shinigami92 Shinigami92 requested a review from a team March 10, 2022 19:43
@Shinigami92 Shinigami92 added p: 2-high Fix main branch and removed p: 2-high Fix main branch labels Mar 10, 2022
@Shinigami92 Shinigami92 added the p: 1-normal Nothing urgent label Mar 15, 2022
@ST-DDT ST-DDT force-pushed the fix/datatype/datatime-seeding branch from e212e54 to c653d75 Compare March 21, 2022 10:03
@ST-DDT ST-DDT force-pushed the fix/datatype/datatime-seeding branch from c653d75 to e383b38 Compare March 21, 2022 13:54
@ST-DDT ST-DDT requested a review from Shinigami92 March 21, 2022 13:54
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Mar 21, 2022
Shinigami92
Shinigami92 previously approved these changes Mar 21, 2022
@ST-DDT ST-DDT requested a review from a team March 21, 2022 13:55
@ST-DDT ST-DDT force-pushed the fix/datatype/datatime-seeding branch from e383b38 to ab59395 Compare March 21, 2022 23:52
Shinigami92
Shinigami92 previously approved these changes Mar 22, 2022
@ST-DDT ST-DDT force-pushed the fix/datatype/datatime-seeding branch from ef583dd to 5b7abf2 Compare March 23, 2022 23:06
@ST-DDT ST-DDT requested a review from Shinigami92 March 23, 2022 23:26
@Shinigami92 Shinigami92 linked an issue Mar 24, 2022 that may be closed by this pull request
@Shinigami92 Shinigami92 requested a review from a team March 24, 2022 07:16
@Shinigami92 Shinigami92 enabled auto-merge (squash) March 24, 2022 08:09
@Shinigami92 Shinigami92 merged commit 7141cd7 into main Mar 24, 2022
@Shinigami92 Shinigami92 deleted the fix/datatype/datatime-seeding branch March 24, 2022 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace new Date().setFullYear(...) with new Date(Date.UTC(...)) datatype.datetime seed might not be working
4 participants