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 daylight shift test #144

Merged
merged 2 commits into from
Feb 9, 2018
Merged

Fix daylight shift test #144

merged 2 commits into from
Feb 9, 2018

Conversation

saifalharthi
Copy link
Contributor

This PR contains a simple fix for the failing test in this issue #141

@randallwhitman
Copy link
Contributor

Would it be simpler to choose a time near noon instead of a time near midnight?
Versus, would choosing a different rather-random epoch number, merely change which timezone suffers the test failure?
@saifalharthi

@saifalharthi
Copy link
Contributor Author

The problem was daily light shifting. It would've been easier to select another random epoch number but we'll still have some problems with respect to other timezones. So, I forced the return date to be converted and formatted to a timezone that has daylight savings and that should work with any epoch number given.
@randallwhitman

@randallwhitman
Copy link
Contributor

So in other words, no what epoch-number we choose, it will be too close to midnight in some time zone.
Thanks Saif.

import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Properties;
import java.util.TimeZone;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance of undoing the spurious re-ordering?

@randallwhitman
Copy link
Contributor

Saif - I reviewed TestGeoJsonSerDe looking if it would likely need the same change. It looks like the timezone was addressed in a different manner in that one. Do you think either way is any better or worse than the other?

@saifalharthi
Copy link
Contributor Author

I will take a look at it

@saifalharthi
Copy link
Contributor Author

I think the approach in TestGeoJsonSerDe should work out fine. I think since it deals with a default timezone offset it will guarantee a more stable time conversion.

@randallwhitman
Copy link
Contributor

OK, sounds good.

@randallwhitman
Copy link
Contributor

Well, if it works, we can merge now and later think about consistency of timezone handling in that test for EsriJson & GeoJson.

@randallwhitman randallwhitman merged commit e3683cd into Esri:master Feb 9, 2018
@randallwhitman randallwhitman added this to the v2.1 milestone Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants