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.

Sign up for GitHub

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

Jump to bottom

[#1601] Convert datetime functionalities to java.time package #1625

Merged
merged 59 commits into from
Feb 21, 2022
Merged

[#1601] Convert datetime functionalities to java.time package #1625

merged 59 commits into from
Feb 21, 2022

Conversation

yhtMinceraft1010X
Copy link
Contributor

@yhtMinceraft1010X yhtMinceraft1010X commented Jan 22, 2022 •
edited by dcshzj
Loading

Fixes #1601

Proposed Commit Message:

SimpleDateFormat, Date and Calendar are not thread-safe, resulting in
tests occasionally failing when multi-threading functionality is used.

Let's use DateTimeFormatter and the java.time package APIs since they
are thread-safe.

@yhtMinceraft1010X yhtMinceraft1010X requested a review from a team January 22, 2022 11:06
@gerhean
Copy link
Contributor

gerhean commented Jan 22, 2022

Tests are failing likely as timezone used is changed. It should not be using the local timezone since I have no idea what timezone github actions runs on.

Timezone logic should be based on config (#1360), else I believe timezone should be ignored.

@gerhean
Copy link
Contributor

gerhean commented Jan 22, 2022 •
edited
Loading

Is it possible to revert the changes to tests since tests do not run in parallel?

@gerhean
Copy link
Contributor

gerhean commented Jan 22, 2022

Actually what's the reason why ZonedDateTime is being used here.

@dcshzj
Copy link
Member

dcshzj commented Jan 22, 2022

Is it possible to revert the changes to tests since tests do not run in parallel?

No, we have to test in parallel as well, otherwise we would not be able to catch concurrency-related issues that occur when users use the software.

Actually what's the reason why ZonedDateTime is being used here.

The timezone uses the user's local timezone on the computer, so that it is something expected when the user runs the software on their computer. GitHub Actions uses UTC+0, unless otherwise specified in a particular test.

This is not supposed to be an easy bug to fix, especially on the timezone issues. I'll do a proper review in a couple of days and provide some guidance on how to address the issue.

dcshzj
dcshzj requested changes Jan 24, 2022
View reviewed changes
src/main/java/reposense/RepoSense.java Outdated Show resolved Hide resolved
src/main/java/reposense/commits/CommitInfoAnalyzer.java Outdated Show resolved Hide resolved
src/main/java/reposense/commits/CommitInfoAnalyzer.java Outdated Show resolved Hide resolved
src/main/java/reposense/git/GitUtil.java Outdated Show resolved Hide resolved
src/main/java/reposense/util/TimeUtil.java Show resolved Hide resolved
src/main/java/reposense/util/TimeUtil.java Outdated Show resolved Hide resolved
src/test/java/reposense/git/GitShowTest.java Outdated Show resolved Hide resolved
src/test/java/reposense/git/GitShowTest.java Outdated Show resolved Hide resolved
src/test/java/reposense/git/GitShowTest.java Outdated Show resolved Hide resolved
src/test/java/reposense/template/GitTestTemplate.java Show resolved Hide resolved
@dcshzj
Copy link
Member

dcshzj commented Jan 24, 2022

The code that added the time zone parameter are in #1346. We use the user's computer time zone (ZoneId.systemDefault()) unless otherwise provided by the user. This time zone will determine the exact instant for the since and until dates (if midnight in Singapore, then will be 4pm in UTC, etc).

Git commit date and time will have the time zone attached to them. We want to convert them into an instant, so that we can compare them with the since and until date instants.

Another tip, to test the time zone stuff, you can change the time zone on your local computer and run the program again to see if it works. GitHub Actions uses UTC+0 time zone.

@yhtMinceraft1010X yhtMinceraft1010X marked this pull request as draft January 25, 2022 14:34
@yhtMinceraft1010X
Copy link
Contributor Author

I'm converting this to draft since this may need more time to fix.

@dcshzj dcshzj changed the title [#1601] Intermittent NumberFormatException [#1601] Convert datetime functionalities to java.time package Feb 8, 2022
@yhtMinceraft1010X yhtMinceraft1010X mentioned this pull request Feb 8, 2022
Closed
@yhtMinceraft1010X
Add comments for boolean
73c3780 
Some boolean statements do not look intuitive.

Let's add comments to explain their purpose.
dcshzj
dcshzj approved these changes Feb 8, 2022
View reviewed changes
Copy link
Member

@dcshzj dcshzj left a comment

Choose a reason for hiding this comment

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

LGTM! Great job on this PR!

@dcshzj dcshzj requested a review from a team February 8, 2022 13:42
This was referenced Feb 13, 2022
Merged
Closed
Closed
@dcshzj
Copy link
Member

dcshzj commented Feb 19, 2022

In the interest of moving this PR along, I will merge this directly. @fzdy1914 Do let me know if you have any additional comments on this PR.

@dcshzj dcshzj merged commit 51cb758 into reposense:master Feb 21, 2022
@github-actions
Copy link
Contributor

The following links are for previewing this pull request:

@yhtMinceraft1010X yhtMinceraft1010X deleted the 1601-fix-intermittent-exceptions branch April 15, 2022 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gerhean gerhean gerhean left review comments

@dcshzj dcshzj dcshzj approved these changes

Assignees
No one assigned
Labels
s.ToMerge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Intermittent NumberFormatException
3 participants
@yhtMinceraft1010X @gerhean @dcshzj