Skip to content

Commit afa170b

Browse files
bitwisemanbitwiseman
committed
Add more tests for rate limit record selection
1 parent 46e3b22 commit afa170b

File tree

8 files changed

+3227
-18
lines changed

8 files changed

+3227
-18
lines changed

‎src/main/java/org/kohsuke/github/GHRateLimit.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,8 @@ Record getRecord(@Nonnull RateLimitTarget rateLimitTarget) {
321321
*/
322322
public static class UnknownLimitRecord extends Record {
323323

324+
private static final long defaultUnknownLimitResetSeconds = Duration.ofSeconds(30).toMillis() / 1000;
325+
324326
/**
325327
* The number of seconds until a {@link UnknownLimitRecord} will expire.
326328
*
@@ -333,7 +335,7 @@ public static class UnknownLimitRecord extends Record {
333335
*
334336
* @see GHRateLimit#getMergedRateLimit(GHRateLimit)
335337
*/
336-
static long unknownLimitResetSeconds = Duration.ofSeconds(30).toMillis() / 1000;
338+
static long unknownLimitResetSeconds = defaultUnknownLimitResetSeconds;
337339

338340
static final int unknownLimit = 1000000;
339341
static final int unknownRemaining = 999999;
@@ -361,8 +363,12 @@ static synchronized Record current() {
361363
return current;
362364
}
363365

366+
/**
367+
* Reset the current UnknownLimitRecord. For use during testing only.
368+
*/
364369
static synchronized void reset() {
365370
current = DEFAULT;
371+
unknownLimitResetSeconds = defaultUnknownLimitResetSeconds;
366372
}
367373
}
368374

‎src/test/java/org/kohsuke/github/GHRateLimitTest.java‎

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import java.io.IOException;
99
import java.time.Duration;
1010
import java.util.Date;
11+
import java.util.HashMap;
1112

1213
import static org.hamcrest.CoreMatchers.equalTo;
1314
import static org.hamcrest.CoreMatchers.sameInstance;
@@ -169,11 +170,13 @@ public void testGitHubRateLimit() throws Exception {
169170
// Verify the requesting a search url updates the search rate limit
170171
assertThat(gitHub.lastRateLimit().getSearch().getRemaining(), equalTo(30));
171172

172-
Object searchResult = gitHub.createRequest()
173+
HashMap<String, Object> searchResult = (HashMap<String, Object>) gitHub.createRequest()
173174
.rateLimit(RateLimitTarget.SEARCH)
174175
.setRawUrlPath(mockGitHub.apiServer().baseUrl()
175-
+ "/search/repositories?q=tetris+language:assembly&sort=stars&order=desc")
176-
.fetch(Object.class);
176+
+ "/search/repositories?q=tetris+language%3Aassembly&sort=stars&order=desc")
177+
.fetch(HashMap.class);
178+
179+
assertThat(searchResult.get("total_count"), equalTo(1918));
177180

178181
assertThat(mockGitHub.getRequestCount(), equalTo(6));
179182

@@ -182,6 +185,21 @@ public void testGitHubRateLimit() throws Exception {
182185
assertThat(gitHub.lastRateLimit().getSearch(), not(sameInstance(headerRateLimit.getSearch())));
183186
assertThat(gitHub.lastRateLimit().getSearch().getRemaining(), equalTo(29));
184187

188+
PagedSearchIterable<GHRepository> searchResult2 = gitHub.searchRepositories()
189+
.q("tetris")
190+
.language("assembly")
191+
.sort(GHRepositorySearchBuilder.Sort.STARS)
192+
.order(GHDirection.DESC)
193+
.list();
194+
195+
assertThat(searchResult2.getTotalCount(), equalTo(1918));
196+
197+
assertThat(mockGitHub.getRequestCount(), equalTo(7));
198+
199+
assertThat(gitHub.lastRateLimit(), not(sameInstance(headerRateLimit)));
200+
assertThat(gitHub.lastRateLimit().getCore(), sameInstance(headerRateLimit.getCore()));
201+
assertThat(gitHub.lastRateLimit().getSearch(), not(sameInstance(headerRateLimit.getSearch())));
202+
assertThat(gitHub.lastRateLimit().getSearch().getRemaining(), equalTo(28));
185203
}
186204

187205
private void verifyRateLimitValues(GHRateLimit previousLimit, int remaining) {
@@ -300,6 +318,7 @@ public void testGitHubEnterpriseDoesNotHaveRateLimit() throws Exception {
300318
// ratelimit() tries not to make additional requests, uses queried rate limit since header not available
301319
Thread.sleep(1500);
302320
assertThat(gitHub.rateLimit(), sameInstance(rateLimit));
321+
assertThat(mockGitHub.getRequestCount(), equalTo(4));
303322

304323
// -------------------------------------------------------------
305324
// Some versions of GHE include header rate limit information, some do not
@@ -353,11 +372,49 @@ public void testGitHubEnterpriseDoesNotHaveRateLimit() throws Exception {
353372
// getRateLimit() uses headerRateLimit if /rate_limit returns a 404
354373
// and headerRateLimit is available and not expired
355374
assertThat(rateLimit, sameInstance(gitHub.lastRateLimit()));
375+
headerRateLimit = rateLimit;
356376

357377
// ratelimit() should prefer headerRateLimit when getRateLimit fails and headerRateLimit is not expired
358378
assertThat(gitHub.rateLimit(), sameInstance(rateLimit));
359379

360380
assertThat(mockGitHub.getRequestCount(), equalTo(6));
381+
382+
// Verify the requesting a search url updates the search rate limit
383+
// Core rate limit record should not change while search is updated.
384+
assertThat(gitHub.lastRateLimit().getSearch(), instanceOf(GHRateLimit.UnknownLimitRecord.class));
385+
assertThat(gitHub.lastRateLimit().getSearch().isExpired(), equalTo(true));
386+
387+
HashMap<String, Object> searchResult = (HashMap<String, Object>) gitHub.createRequest()
388+
.rateLimit(RateLimitTarget.SEARCH)
389+
.setRawUrlPath(mockGitHub.apiServer().baseUrl()
390+
+ "/search/repositories?q=tetris+language%3Aassembly&sort=stars&order=desc")
391+
.fetch(Object.class);
392+
393+
assertThat(searchResult.get("total_count"), equalTo(1918));
394+
395+
assertThat(mockGitHub.getRequestCount(), equalTo(7));
396+
397+
assertThat(gitHub.lastRateLimit(), not(sameInstance(headerRateLimit)));
398+
assertThat(gitHub.lastRateLimit().getCore(), sameInstance(headerRateLimit.getCore()));
399+
assertThat(gitHub.lastRateLimit().getSearch(), not(sameInstance(headerRateLimit.getSearch())));
400+
assertThat(gitHub.lastRateLimit().getSearch().getRemaining(), equalTo(29));
401+
402+
PagedSearchIterable<GHRepository> searchResult2 = gitHub.searchRepositories()
403+
.q("tetris")
404+
.language("assembly")
405+
.sort(GHRepositorySearchBuilder.Sort.STARS)
406+
.order(GHDirection.DESC)
407+
.list();
408+
409+
assertThat(searchResult2.getTotalCount(), equalTo(1918));
410+
411+
assertThat(mockGitHub.getRequestCount(), equalTo(8));
412+
413+
assertThat(gitHub.lastRateLimit(), not(sameInstance(headerRateLimit)));
414+
assertThat(gitHub.lastRateLimit().getCore(), sameInstance(headerRateLimit.getCore()));
415+
assertThat(gitHub.lastRateLimit().getSearch(), not(sameInstance(headerRateLimit.getSearch())));
416+
assertThat(gitHub.lastRateLimit().getSearch().getRemaining(), equalTo(28));
417+
361418
}
362419

363420
@Test

‎src/test/java/org/kohsuke/github/GitHubStaticTest.java‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -65,14 +65,14 @@ public void timeRoundTrip() throws Exception {
6565
@Test
6666
public void testGitHubRateLimitShouldReplaceRateLimit() throws Exception {
6767

68-
GHRateLimit.UnknownLimitRecord.unknownLimitResetSeconds = 5;
6968
GHRateLimit.UnknownLimitRecord.reset();
69+
GHRateLimit.UnknownLimitRecord.unknownLimitResetSeconds = 5;
7070

7171
GHRateLimit.Record unknown0 = GHRateLimit.UnknownLimitRecord.current();
7272

73+
Thread.sleep(1500);
7374
GHRateLimit.UnknownLimitRecord.reset();
74-
75-
Thread.sleep(2000);
75+
GHRateLimit.UnknownLimitRecord.unknownLimitResetSeconds = 5;
7676

7777
// For testing, we create an new unknown.
7878
GHRateLimit.Record unknown1 = GHRateLimit.UnknownLimitRecord.current();
@@ -85,7 +85,7 @@ public void testGitHubRateLimitShouldReplaceRateLimit() throws Exception {
8585
sameInstance(unknown0));
8686

8787
// Sleep to make different created time
88-
Thread.sleep(2000);
88+
Thread.sleep(1500);
8989

9090
// To reduce object creation: There is only one valid Unknown record at a time.
9191
assertThat("Unknown current should should limit the creation of new unknown records",
@@ -103,7 +103,7 @@ public void testGitHubRateLimitShouldReplaceRateLimit() throws Exception {
103103
GHRateLimit.Record recordExpired1 = new GHRateLimit.Record(10, 10, epochSeconds + 2L);
104104

105105
// Sleep to make expired and different created time
106-
Thread.sleep(3000);
106+
Thread.sleep(4000);
107107

108108
GHRateLimit.Record recordWorst = new GHRateLimit.Record(Integer.MAX_VALUE, Integer.MAX_VALUE, Long.MIN_VALUE);
109109
GHRateLimit.Record record00 = new GHRateLimit.Record(10, 10, epochSeconds + 10L);
@@ -123,21 +123,18 @@ public void testGitHubRateLimitShouldReplaceRateLimit() throws Exception {
123123
unknownExpired1.currentOrUpdated(unknownExpired0),
124124
sameInstance(unknownExpired1));
125125

126-
assertThat(
127-
"Expired unknown should not be replaced by expired earlier normal record, regardless of created or reset time",
126+
assertThat("Expired unknown should not be replaced by expired earlier normal record",
128127
unknownExpired0.currentOrUpdated(recordExpired0),
129128
sameInstance(unknownExpired0));
130-
assertThat(
131-
"Expired normal record should not be replace an expired earlier unknown record, regardless of created or reset time",
129+
assertThat("Expired normal record should not be replaced an expired earlier unknown record",
132130
recordExpired0.currentOrUpdated(unknownExpired0),
133131
sameInstance(recordExpired0));
134132

135-
assertThat(
136-
"Expired unknown should be replaced by expired later normal record, regardless of created or reset time",
133+
assertThat("Expired unknown should be replaced by expired later normal record",
137134
unknownExpired0.currentOrUpdated(recordExpired1),
138135
sameInstance(recordExpired1));
139136
assertThat(
140-
"Expired later normal record should not be replace an expired unknown record, regardless of created or reset time",
137+
"Expired later normal record should not be replaced an expired unknown record, regardless of created or reset time",
141138
recordExpired1.currentOrUpdated(unknownExpired0),
142139
sameInstance(recordExpired1));
143140

@@ -151,7 +148,7 @@ public void testGitHubRateLimitShouldReplaceRateLimit() throws Exception {
151148
assertThat("Valid unknown should replace an expired normal record",
152149
recordExpired1.currentOrUpdated(unknown0),
153150
sameInstance(unknown0));
154-
assertThat("Expired normal record should not replace a valid unknown record",
151+
assertThat("Valid unknown record should not be replaced by expired normal record",
155152
unknown0.currentOrUpdated(recordExpired1),
156153
sameInstance(unknown0));
157154

0 commit comments

Comments
 (0)