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 Date for dynamic timezones #2877

Merged
merged 2 commits into from May 5, 2023
Merged

Fix Date for dynamic timezones #2877

merged 2 commits into from May 5, 2023

Conversation

jedel1043
Copy link
Member

This Pull Request fixes #2874.

It changes the following:

  • Changes our HostHooks API to correctly handle dynamic timezones.

@jedel1043 jedel1043 added builtins PRs and Issues related to builtins/intrinsics API labels Apr 27, 2023
@jedel1043 jedel1043 added this to the v0.17.0 milestone Apr 27, 2023
@github-actions
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 94,593 94,593 0
Passed 73,192 73,192 0
Ignored 17,532 17,532 0
Failed 3,869 3,869 0
Panics 0 0 0
Conformance 77.38% 77.38% 0.00%

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #2877 (f01e312) into main (7d2be7e) will increase coverage by 0.30%.
The diff coverage is 80.72%.

@@            Coverage Diff             @@
##             main    #2877      +/-   ##
==========================================
+ Coverage   50.91%   51.21%   +0.30%     
==========================================
  Files         419      427       +8     
  Lines       41883    42503     +620     
==========================================
+ Hits        21324    21768     +444     
- Misses      20559    20735     +176     
Impacted Files Coverage Δ
boa_engine/src/builtins/date/mod.rs 92.88% <78.57%> (+0.66%) ⬆️
boa_engine/src/builtins/date/utils.rs 95.34% <83.33%> (-3.45%) ⬇️
boa_engine/src/context/hooks.rs 78.78% <100.00%> (+3.78%) ⬆️

... and 95 files with indirect coverage changes

@Razican
Copy link
Member

Razican commented Apr 28, 2023

I think this will be a breaking change for @lastmjs, so I'm pinging to make sure this works for them.

I'll test this later today, since I was having the mentioned issue.

@lastmjs
Copy link
Contributor

lastmjs commented Apr 28, 2023

I'll see if I can test this soon

@Razican
Copy link
Member

Razican commented Apr 28, 2023

I'm getting the following test error with this branch:

failures:

---- builtins::date::tests::date_proto_get_timezone_offset stdout ----
thread 'builtins::date::tests::date_proto_get_timezone_offset' panicked at 'assertion failed: (left == right)
left: Integer(-60),
right: Integer(-120):

Test case 2:

    new Date('1975-08-19T23:15:30+07:00').getTimezoneOffset()

In boa_engine/src/builtins/date/tests.rs:251:5
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

failures:
builtins::date::tests::date_proto_get_timezone_offset

test result: FAILED. 678 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.16s

error: test failed, to rerun pass -p boa_engine --lib

@jedel1043
Copy link
Member Author

@Razican Pushed a change to fix the test, let me know if it works for you.

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

It works now! Let's wait for @lastmjs confirmation, and we can merge it :)

@lastmjs
Copy link
Contributor

lastmjs commented May 1, 2023

I would like to test this today, I'll let you know

@lastmjs
Copy link
Contributor

lastmjs commented May 4, 2023

Sorry for the delay, works for me!

@HalidOdat HalidOdat added this pull request to the merge queue May 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 4, 2023
@jedel1043 jedel1043 added this pull request to the merge queue May 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 5, 2023
@Razican Razican added this pull request to the merge queue May 5, 2023
Merged via the queue into main with commit debbb91 May 5, 2023
9 checks passed
@bors bors bot deleted the fix-date-timezones branch May 5, 2023 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

boa_engine builtin::date unit tests fail for DST timezones
4 participants