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 dst test failure #7649

Merged
merged 6 commits into from
Aug 31, 2023
Merged

fix dst test failure #7649

merged 6 commits into from
Aug 31, 2023

Conversation

GregoryTravis
Copy link
Contributor

@GregoryTravis GregoryTravis commented Aug 23, 2023

Pull Request Description

Fixes a test failure that depends on the date on which Daylight Saving Time changes.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@GregoryTravis GregoryTravis added the CI: No changelog needed Do not require a changelog entry for this PR. label Aug 23, 2023
@GregoryTravis GregoryTravis marked this pull request as ready for review August 23, 2023 18:35
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

I believe that using Time_Zone.system makes the tests non-portable and should be eliminated. This change is thus a move towards the right direction as it removes usage of Time_Zone.system in one problematic (for Greg)Test.specify.

Clearly helps @GregoryTravis and the CI is green: let's integrate!

@@ -783,22 +783,22 @@ spec_with name create_new_datetime parse_datetime nanoseconds_loss_in_precision=
t1.date_diff t2 Time_Period.Day . should_equal 32

t1.date_diff t2 Time_Period.Hour . should_equal 770
t1.date_diff (Date_Time.new 2021 11 3 12 15 0) Time_Period.Hour . should_equal 2
t1.date_diff (Date_Time.new 2021 11 3 12 15 0 zone=Time_Zone.utc) Time_Period.Hour . should_equal 2
Copy link
Member

@JaroslavTulach JaroslavTulach Aug 24, 2023

Choose a reason for hiding this comment

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

I am surprised we are using Date_Time.new here and not create_new_datetime. That means we are testing only Enso implementation, not interop with other languages.

Copy link
Member

Choose a reason for hiding this comment

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

This is deliberate.

If we use create_new_datetime everywhere, we test how values from one language tested at given time interact with each other. By using Date_Time.new for the argument and still using create_new_datetime for the receiver (self = t1), we test how an Enso value interoperates with a value coming from another language - i.e. we check mixing of values from different languages with each other - which also needs to be supported as the values should all be interchangeable regardless of what language they come from.

I don't remember if this was added because there bugs with that or if it was just to ensure we test a wider range of scenarios, but surely it was done deliberately to test more interesting 'mixing' cases.

Comment on lines +770 to +771
t1 = create_new_datetime 2021 11 3 10 15 0 zone=Time_Zone.utc
t2 = create_new_datetime 2021 12 5 12 30 20 zone=Time_Zone.utc
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if instead of this, we should just make the default zone be utc in the test builders.

i.e.

Index: test/Tests/src/Data/Time/Date_Time_Spec.enso
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/Tests/src/Data/Time/Date_Time_Spec.enso b/test/Tests/src/Data/Time/Date_Time_Spec.enso
--- a/test/Tests/src/Data/Time/Date_Time_Spec.enso	(revision eb1550014b3c7120dc57ab111921eb7d18c2d5fe)
+++ b/test/Tests/src/Data/Time/Date_Time_Spec.enso	(date 1692868316355)
@@ -847,7 +847,7 @@
 
     Date_Part_Spec.spec name create_new_datetime
 
-js_datetime year month=1 day=1 hour=0 minute=0 second=0 nanosecond=0 zone=Time_Zone.system =
+js_datetime year month=1 day=1 hour=0 minute=0 second=0 nanosecond=0 zone=Time_Zone.utc =
     Panic.catch Any (js_datetime_with_zone year month day hour minute second nanosecond zone) (err -> Error.throw (Time_Error.Error err.payload))
 
 # This ensures that date returned by javascript has the right timezone specified by the zone parameter.
@@ -914,10 +914,10 @@
     else:
         return datetime.datetime(year, month, day, hour, minute, second, microseconds, Zone(zone, zone_name))
 
-enso_datetime year month=1 day=1 hour=0 minute=0 second=0 nanosecond=0 zone=Time_Zone.system =
+enso_datetime year month=1 day=1 hour=0 minute=0 second=0 nanosecond=0 zone=Time_Zone.utc =
     Date_Time.new year month day hour minute second nanosecond=nanosecond zone=zone
 
-java_datetime year month=1 day=1 hour=0 minute=0 second=0 nanosecond=0 zone=Time_Zone.system =
+java_datetime year month=1 day=1 hour=0 minute=0 second=0 nanosecond=0 zone=Time_Zone.utc =
     Panic.catch Any (ZonedDateTime.of year month day hour minute second nanosecond zone) (err -> Error.throw (Time_Error.Error <| err.payload.getMessage))
 
 maybe_parse_java_zoned text pattern=Nothing =

What do you think? This way new tests will be more likely to be written correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try it, but the test is currently failing for me on develop, for other reasons: #7655

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using this exact diff, I get 18 failures.

@GregoryTravis GregoryTravis added the CI: Ready to merge This PR is eligible for automatic merge label Aug 29, 2023
@GregoryTravis GregoryTravis added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Aug 30, 2023
@mergify mergify bot merged commit 5468951 into develop Aug 31, 2023
@mergify mergify bot deleted the wip/gmt/dst-test-failure branch August 31, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants