Fix `from_timestamp_millis` implementation and add more tests #885

esheppa · 2022-11-20T06:40:20Z

Bug reported here: #884

djc

Thanks for fixing this!

src/naive/datetime/tests.rs

djc · 2022-11-20T17:50:57Z

src/naive/datetime/tests.rs

+    assert_eq!(naive_datetime.unwrap().year(), 2022);
+    assert_eq!(naive_datetime.unwrap().month(), 9);
+    assert_eq!(naive_datetime.unwrap().day(), 11);
+    assert_eq!(naive_datetime.unwrap().hour(), 18);
+    assert_eq!(naive_datetime.unwrap().minute(), 34);
+    assert_eq!(naive_datetime.unwrap().second(), 48);
+    assert_eq!(naive_datetime.unwrap().timestamp_subsec_nanos(), 0);


Maybe cover most of the fields with a single assert_eq!() against a string-encoded timestamp? Would be a little more compact and readable.

good call - I've switched to comparing against a formatted string

@djc

Just a general cleanup of the `Date` builtin to use slightly better patterns and to fix our warnings about deprecated functions. About the regressed tests. It seems to be a `chrono` bug, so I opened up an issue (chronotope/chrono#884) for it and they've already opened a PR fixing it (chronotope/chrono#885). However, while checking out the remaining failing tests, I realized there's a more fundamental limitation with the library. Currently, [`chrono`](https://github.com/chronotope/chrono) specifies: > Date types are limited in about +/- 262,000 years from the common epoch. While the [ECMAScript spec](https://tc39.es/ecma262/#sec-time-values-and-time-range) says: > The smaller range supported by a time value as specified in this section is approximately -273,790 to 273,790 years relative to 1970. The range allowed by the spec is barely outside of the range supported by `chrono`! This is why the remaining `Date` tests fail. Seeing that, I would like to ping @djc and @esheppa (the maintainers of `chrono`) to ask if it would be feasible to add a feature, akin to the `large-dates` feature from the `time` crate, that expands the supported range of `chrono`. EDIT: Filed chronotope/chrono#886

src/naive/datetime/tests.rs

esheppa · 2022-11-23T02:09:57Z

Just fixed the issue on MSRV, you may want to re-review

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [chrono](https://github.com/chronotope/chrono) | dependencies | patch | `0.4.23` -> `0.4.24` | --- ### Release Notes <details> <summary>chronotope/chrono</summary> ### [`v0.4.24`](https://github.com/chronotope/chrono/releases/tag/v0.4.24): 0.4.24 [Compare Source](chronotope/chrono@v0.4.23...v0.4.24) This is a small maintenance release with accumulated fixes and improvements. - Fix doc on `Days::new()` to refer to days, not months ([#874](chronotope/chrono#874), thanks to [@brotskydotcom](https://github.com/brotskydotcom)) - Clarify out of range value for `from_timestamp_opt()` ([#879](chronotope/chrono#879), thanks to [@xmo-odoo](https://github.com/xmo-odoo)) - Add `format_localized()` for `NaiveDate` ([#881](chronotope/chrono#881), thanks to [@mseele](https://github.com/mseele)) - Fix bug in `Add`/`Sub` `Days`, add tests with DST timezone ([#878](chronotope/chrono#878)) - Make `NaiveTime::MIN` public ([#890](chronotope/chrono#890)) - Fix `from_timestamp_millis()` implementation and add more tests ([#885](chronotope/chrono#885)) - Fix typo in docstrings ([#897](chronotope/chrono#897), thanks to [@dandxy89](https://github.com/dandxy89)) - Add test proving that [#903](chronotope/chrono#903) is fixed in 0.4.x head ([#905](chronotope/chrono#905), thanks to [@umanwizard](https://github.com/umanwizard)) - Add `from_timestamp_micros()` function ([#906](chronotope/chrono#906), thanks to [@umanwizard](https://github.com/umanwizard)) - Check cargo-deny in CI ([#909](chronotope/chrono#909)) - Derive `Hash` for most pub types that also derive `PartialEq` ([#938](chronotope/chrono#938), thanks to [@bruceg](https://github.com/bruceg)) - Update deprecated methods in `from_utc()` example ([#939](chronotope/chrono#939), thanks to [@greg-el](https://github.com/greg-el)) - Fix panic in `DateTime::checked_add_days()` ([#942](chronotope/chrono#942), thanks to [@Ekleog](https://github.com/Ekleog)) - More documentation for dates before 1 BCE or after 9999 CE ([#950](chronotope/chrono#950), thanks to [@cgit](https://github.com/cgit)) - Improve `FixedOffset` docs ([#953](chronotope/chrono#953), thanks to [@klnusbaum](https://github.com/klnusbaum)) - Add chrono-fuzz to CI and update its libfuzzer-sys dependency ([#968](chronotope/chrono#968), thanks to [@LingMan](https://github.com/LingMan)) - Fixes to parsing and calculation of week numbers ([#966](chronotope/chrono#966), thanks to [@raphaelroosz](https://github.com/raphaelroosz)) - Make iana-time-zone a target specific dependency ([#980](chronotope/chrono#980), thanks to [@krtab](https://github.com/krtab)) - Make eligible functions `const` ([#984](chronotope/chrono#984), thanks to [@tormeh](https://github.com/tormeh)) Thanks to all contributors from the chrono team, [@esheppa](https://github.com/esheppa) and [@djc](https://github.com/djc). </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1815 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

esheppa mentioned this pull request Nov 20, 2022

Incorrect conversion of TimeZone::from_utc_datetime #884

Closed

djc reviewed Nov 20, 2022

View reviewed changes

jedel1043 mentioned this pull request Nov 20, 2022

[Merged by Bors] - Refactor the Date builtin boa-dev/boa#2449

Closed

esheppa force-pushed the fix-timestamp-from-millis branch 3 times, most recently from 7b2db6d to aff4f3c Compare November 23, 2022 00:25

djc reviewed Nov 23, 2022

View reviewed changes

src/naive/datetime/tests.rs Outdated Show resolved Hide resolved

esheppa force-pushed the fix-timestamp-from-millis branch from aff4f3c to 65c0c8a Compare November 23, 2022 00:55

djc approved these changes Nov 23, 2022

View reviewed changes

esheppa force-pushed the fix-timestamp-from-millis branch from 65c0c8a to 9fc2f06 Compare November 23, 2022 02:09

fix bug and add more test cases

9d0da95

esheppa force-pushed the fix-timestamp-from-millis branch from 9fc2f06 to 9d0da95 Compare November 23, 2022 02:35

djc approved these changes Nov 23, 2022

View reviewed changes

djc merged commit ff370ae into 0.4.x Nov 23, 2022

djc deleted the fix-timestamp-from-millis branch November 23, 2022 06:25

esheppa mentioned this pull request Dec 13, 2022

NaiveDateTime::from_timestamp_millis(-1000) does not compare equal to NaiveDateTime::from_timestamp_opt(-1, 0) #903

Closed

djc mentioned this pull request Jan 3, 2023

0.4.24 release planning #929

Closed

pitdicker mentioned this pull request Jun 4, 2023

MIN_UTC's timestamp_millis is too large to be converted back into datetime. #934

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `from_timestamp_millis` implementation and add more tests #885

Fix `from_timestamp_millis` implementation and add more tests #885

esheppa commented Nov 20, 2022

djc left a comment

djc Nov 20, 2022

esheppa Nov 23, 2022

esheppa commented Nov 23, 2022

Fix from_timestamp_millis implementation and add more tests #885

Fix from_timestamp_millis implementation and add more tests #885

Conversation

esheppa commented Nov 20, 2022

djc left a comment

Choose a reason for hiding this comment

djc Nov 20, 2022

Choose a reason for hiding this comment

esheppa Nov 23, 2022

Choose a reason for hiding this comment

esheppa commented Nov 23, 2022

Fix `from_timestamp_millis` implementation and add more tests #885

Fix `from_timestamp_millis` implementation and add more tests #885