Skip to content
This repository was archived by the owner on Jun 18, 2024. It is now read-only.

Add unit tests for util classes (DateTimeUtils, TimeZoneUtils)#348

Merged
serious6 merged 1 commit intoOfficeDev:masterfrom
vbauer:ut-for-uc
Jun 26, 2015
Merged

Add unit tests for util classes (DateTimeUtils, TimeZoneUtils)#348
serious6 merged 1 commit intoOfficeDev:masterfrom
vbauer:ut-for-uc

Conversation

@vbauer
Copy link
Copy Markdown
Contributor

@vbauer vbauer commented Jun 14, 2015

No description provided.

@azurecla
Copy link
Copy Markdown

Hi @vbauer, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, AZPRBOT;

@serious6
Copy link
Copy Markdown
Member

looks good 👍

@vboctor
Copy link
Copy Markdown
Contributor

vboctor commented Jun 16, 2015

Looks good. Would be nice to do some non-UTC tests and potentially support case-insensitive lookups in the timezone hashmap.

@vbauer
Copy link
Copy Markdown
Contributor Author

vbauer commented Jun 17, 2015

It looks like TimeZone class doesn't support case-insensitive identifiers (see TimeZone.getTimeZone) and method TimeZoneUtils.getMicrosoftTimeZoneName works with it.

I'll add some positive and negative tests for lookup.

@vbauer
Copy link
Copy Markdown
Contributor Author

vbauer commented Jun 17, 2015

PR was updated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it make sense to keep the ones that are out of date? When are these useful?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@vboctor I don't know (it isn't my code), I've just formatted length of this line (it was too long).

@vbauer
Copy link
Copy Markdown
Contributor Author

vbauer commented Jun 19, 2015

Thx for CR, PR was updated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I found that it depends on env. setup. Is it OK?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shouldnt this be a little against design of unit-test to assert one or another?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I also don't like this, but as I wrote method TimeZone.getTimeZone depends on tzdata (in JVM):

  • If you have timezone "Antarctica/Troll" in tzdata, than you'll get "null".
  • If you haven't it, than you'll have "UTC".

Do you have any idea how to improve it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@vbauer wouldnt it be a solution to the problem if we just pick a timezone that def. will be in tzdata? Or is this just random? And why do you think this timezone will not be referenced on travis? If this can cause problems please add a comment to the test. This could ref. to tzdata-versions for instance.

@vbauer
Copy link
Copy Markdown
Contributor Author

vbauer commented Jun 25, 2015

PR was update. I think it will be enough to have test with "Africa/Abidjan" timezone.

serious6 added a commit that referenced this pull request Jun 26, 2015
Add unit tests for util classes (DateTimeUtils, TimeZoneUtils) and minor improvements
@serious6 serious6 merged commit f06c90b into OfficeDev:master Jun 26, 2015
@serious6
Copy link
Copy Markdown
Member

@vbauer thanks for your contribution

@serious6 serious6 added this to the 2.0 milestone Jul 9, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants