Improve wait handing in abuse retry

[holly-cummins]

Fixes #1909.

While I was looking at #1909, I noticed that this code in my earlier PR is a bit hard to follow:

return Math.max(1000, Long.parseLong(v) * 1000);




I think it might have been copied from [this line] (https://github.com/hub4j/github-api/blame/768c7154bdb84e775dfafea6b0cb27fa57d835c7/src/main/java/org/kohsuke/github/GitHubRateLimitHandler.java#L80) in GitHubRateLimitHandler:

return Math.max(1000, Long.parseLong(v) * 1000 - System.currentTimeMillis());


That second piece of code also uses System.currentTimeMillis(), which as pointed out in #1909 may not be accurate. The class was changed in a reformat, but apart from that hasn’t been changed for a while, so I think the problem is an old one. I didn't touch that class in this change, but we should go back and update it too.

So this is what I’ve done:

• Strengthened the assertions in the abuse limit handler test, to properly exercise the logic for calculating wait periods, since I got confused at a few points about what I should be expecting :)
• I’ve swapped all those 1000s for a constant
• I’ve applied the Math.min() to both paths (number and date)

[holly-cummins]

Oh gosh, I thought reset my branch to the latest upstream before starting work, but apparently I reset to some other entirely different branch instead. Will sort it out.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.09%. Comparing base (768c715) to head (b22f346).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #1971   +/-   ##
=========================================
  Coverage     83.08%   83.09%           
- Complexity     2328     2329    +1     
=========================================
  Files           231      231           
  Lines          7164     7168    +4     
  Branches        376      377    +1     
=========================================
+ Hits           5952     5956    +4     
  Misses          974      974           
  Partials        238      238           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[holly-cummins]

The code coverage report is interesting. My code handles the case where the "Date" field isn't set, even though all the example responses from GitHub do include this header. I could fix the coverage warning by removing the guard, and assuming that "Date" is always present – but I don't think that would improve the code quality. :)

I could also fix the warning by adding a test for that scenario, but I'm not sure it's the best use of time, since it's an unlikely scenario which the code already guards against. What do you think, @bitwiseman?

[holly-cummins]

I'm not convinced it's adding value, since it's trivial code and an unlikely scenario, but I've added the test for the "missing date header" case. :)

[bitwiseman]

Sorry, I thought I'd responded to say that a missing Date header is so rare that I'd be fine not covering it. Thanks for do it!