Skip to content

fix: datatype.datetime should use static boundaries#343

Merged
Shinigami92 merged 2 commits into
mainfrom
fix/datatype/datatime-seeding
Mar 24, 2022
Merged

fix: datatype.datetime should use static boundaries#343
Shinigami92 merged 2 commits into
mainfrom
fix/datatype/datatime-seeding

Conversation

@ST-DDT

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

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member

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

@ST-DDT

ST-DDT commented Jan 29, 2022

Copy link
Copy Markdown
Member Author

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

ST-DDT commented Jan 29, 2022

Copy link
Copy Markdown
Member Author

This needs to be rebased after #344 is merged.

@ST-DDT

ST-DDT commented Jan 29, 2022

Copy link
Copy Markdown
Member Author

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

codecov Bot commented Feb 6, 2022

Copy link
Copy Markdown

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

@pkuczynski pkuczynski left a comment

Copy link
Copy Markdown
Member

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

4 participants