Repository navigation
Serve push notification test image from MockWebServer - #1097
franco-zalamena-iterable wants to merge 2 commits into
Conversation
testNotificationImage pointed attachment-url at a live logo on assets.iterable.com. That asset now returns 404, so getInputStream() throws, no BigPictureStyle is attached, and the EXTRA_PICTURE assertion fails on every branch including master. Serve the existing 350x150.png test resource locally instead so the test no longer depends on a CDN. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
IterableRequestTask resolves the endpoint from a static override on every attempt and schedules retries on a static main-looper Handler up to RETRY_DELAY_MS * MAX_RETRY_COUNT ahead. testResponseCode500 asserted that retry #2 happened and returned, leaving retries 3-5 pending; those fired after tearDown and re-resolved to the next test's MockWebServer, consuming the response it had enqueued. The default queue dispatcher blocks on an empty queue, so the victim hung until its latch timed out - usually testResponseCode400WithoutMessage - and a stray response reaching a finished test's assertion in a callback crashed the instrumentation process. Serve responses from a dispatcher that repeats the last scripted response so a stray request is harmless, and end the 500 retry chain with a success so it cannot outlive the test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| final byte[] imageBytes = getResourceBytes("350x150.png"); | ||
| imageServer = new MockWebServer(); | ||
| // Every notification build() re-downloads the attachment, so serve the image on | ||
| // every request instead of enqueuing a fixed number of responses. | ||
| imageServer.setDispatcher(new Dispatcher() { | ||
| @Override | ||
| public MockResponse dispatch(RecordedRequest request) { | ||
| return new MockResponse() | ||
| .setHeader("Content-Type", "image/png") | ||
| .setBody(new Buffer().write(imageBytes)); | ||
| } | ||
| }); | ||
| imageServer.start(); | ||
|
|
||
| itbl_image = "{\"templateId\":1,\"campaignId\":1,\"messageId\":\"11111111111111111111111111111111\",\"isGhostPush\":false,\"attachment-url\":\"" + imageServer.url("/push-image.png") + "\"}"; |
There was a problem hiding this comment.
Also iterableapi/src/androidTest/java/com/iterable/iterableapi/IterableApiResponseTest.java ~L120: this branch does not apply onto current master
What the code does: The branch serves the push image from a MockWebServer started in every IterableNotificationTest setUp, and IterableApiResponseTest still calls new IterableRequestTask().execute(request) (for example ~L120 and ~L318). git merge-tree against master (79444469) reports both files changed on both sides.
What the spec says: Commit 49f637c6 says to stop depending on the CDN and serve 350x150.png locally. Commit 080825af says a 500 retry left on the static main-looper Handler must not hit the next test’s server.
Why it conflicts: master already points testNotificationImage at that PNG via ClassLoader.getResource in #1093 (IterableNotificationTest.java ~L116–122). #1096 removed IterableRequestTask.execute; the class is a Runnable, and the instrumentation test calls dispatchRequest → IterableRequestDispatcher.online().execute. master’s testResponseCode500 (~L275–285) still enqueues five 500s and returns after the second takeRequest, while retryRequestWithDelay (~L419–421) still schedules later attempts. The leak this PR describes is still on master, and this patch does not compile against that API.
Suggested action: Rebase onto master. Keep #1093’s classpath image URL. Reapply only the retry fix on top of dispatchRequest.
| server.setDispatcher(new Dispatcher() { | ||
| @Override | ||
| public MockResponse dispatch(RecordedRequest request) { | ||
| synchronized (scriptLock) { | ||
| MockResponse next = scriptedResponses.poll(); | ||
| if (next != null) { | ||
| lastServedResponse = next; | ||
| } else if (lastServedResponse == null) { | ||
| lastServedResponse = new MockResponse().setResponseCode(200).setBody("{}"); | ||
| } | ||
| return lastServedResponse; | ||
| } |
There was a problem hiding this comment.
also ~L309–323: the 500 chain can still outlive the test, and a stray request still consumes the next script
What the code does: testResponseCode500 stubs 500, 500, then 200, then returns after the second takeRequest. The dispatcher poll()s a scripted response first and, only when the deque is empty, returns the same lastServedResponse instance. tearDown does not drain the main looper.
What the spec says: Commit 080825af says to end the chain with a success so it cannot outlive the test, and to repeat the last response so a stray request is harmless.
Why it conflicts: Attempt 3 is the 200, scheduled with delay 0 (retryCount > 2 is false for the second 500) and never awaited, so it can run after overrideURLEndpointPath points at the next server. If that test has already called stubResponse, poll() takes its scripted response. Repeating only happens when the deque is empty. OkHttp MockWebServer 4.9 writes that body with throttledTransfer, which reads the Buffer until it is empty, so a second use of the same MockResponse has an empty body.
Suggested action: On the rebase, wait until the terminal success has been received before the test returns, and have the dispatcher return a new copy of the fallback response on every call so it neither steals a scripted response nor serves a drained body.
2 participants
Fixing test failing on CI
No customer facing changes