Fix failing test case in TimeZoneUtilsTest#393
Fix failing test case in TimeZoneUtilsTest#393cfraser wants to merge 1 commit intoOfficeDev:masterfrom
Conversation
* added test to verify that ```getMicrosoftTimeZoneName``` returns null when passed an case insensitive timezone id.
|
Hi @cfraser, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution! TTYL, AZPRBOT; |
|
this might be a duplicate of #392 ? |
|
#392 appears to just wrap the underlying issue in a try/catch. While that will mitigate the Assert, it doesn't test that the expected value is returned. |
|
I don't understand why the test is failing on TravisCI. The string Am I missing some environment variables, or some other patch that is not part of the master branch? Is it possible something was put in place to pass this test previously given it was not passing a fresh clone, There's no record of the TravisCI automation result when the test was added in #348 that I can see in the history. Please excuse all the questions, this is my first submission to any OSS project. I am planning on submitting some Calendar fixes with tests shortly. |
|
this is really strange. I will try to solve this later on. |
|
can you try run that build on your local machine? |
|
|
Sorry for all the back and forth on this. It appeared trivial last night. |
There was a problem hiding this comment.
I actually dont see what this test is about? Do you want to test TimeZone.getTimeZone(...) ? Since this is a java.util and no ews-java-api class testing if it will return null on case-insensitive is not related to our project. This test should be removed.
There was a problem hiding this comment.
btw this test also fails when I execute it with intelliJ on the local repo.
There was a problem hiding this comment.
To be honest, this isn't much of a test. It should probably just be removed from the test class. All it does is validate that the string isn't a key in TimeZoneUtils.createOlsonTimeZoneToMsMap(). It should return null since it's not in the map, but don't have a good explaination for why that is not the case on the build machine, and on your local system.
Unfortunately I can't reproduce the failure so I guess I'm going to withdraw the PR. It's strange to me that #392 is also"fixing" this issue and it also can't be reproduced by the Travis build.
C'est la vie.
There was a problem hiding this comment.
Ok. Nevertheless thanks for your contribution.
There was a problem hiding this comment.
I'll try with Java 8. I'm using 7.
I apologize for all the back on forth on this very trivial commit. I don't want to be wasting your time.
There was a problem hiding this comment.
I'll keep trying to contribute. 👍
I just have to get a senior director to sign the CLA, and the other PR should be unblocked.
There was a problem hiding this comment.
I am able to reproduce this test failing when I use Java 8. It appears that TimeZone defaults to UTC when the time zone id cannot be resolved as documented in the javadocs of getTimeZone(String ID)
/**
* Gets the <code>TimeZone</code> for the given ID.
*
* @param ID the ID for a <code>TimeZone</code>, either an abbreviation
* such as "PST", a full name such as "America/Los_Angeles", or a custom
* ID such as "GMT-8:00". Note that the support of abbreviations is
* for JDK 1.1.x compatibility only and full names should be used.
*
* @return the specified <code>TimeZone</code>, or the GMT zone if the given ID
* cannot be understood.
*/
public static synchronized TimeZone getTimeZone(String ID) {
return getTimeZone(ID, true);
}
.
.
.
private static TimeZone getTimeZone(String ID, boolean fallback) {
TimeZone tz = ZoneInfo.getTimeZone(ID);
if (tz == null) {
tz = parseCustomTimeZone(ID);
if (tz == null && fallback) {
tz = new ZoneInfo(GMT_ID, 0);
}
}
return tz;
}
It's worth documenting in case someone else runs into this in the future.
|
@cfraser please fix the build errors. |
3 participants
getMicrosoftTimeZoneNamereturns null when passed a case insensitive timezone id.