Follow-up to Pocket background fetch

[dnarcese]

Follow-up tasks to #2223

Vision statement / What / Requirements

The tasks that were not finished in the original PR:

• Remove remaining old Pocket foreground fetch code

  • Rename PocketEndpoint?
  • Remove PocketRepoCache and comment why it's not necessary (b/c we only content in onStart)
  • Remove other unused code

• Clean up comments in scheduler

• Notify team

  • We only update for users who use the app; updates only scheduled if user uses app that day (what implications might product and UX care about?)
  • Users who never background the app (i.e. for whom screensaver is disabled, or constantly use the app) will never get updates

• Documentation

  • Email + code comments that explains how the fetch works
  • WorkManager edge cases documented in shared docs

• Automated tests

  • No API key
  • Non en-US locales
  • Failure to hit the server == retry in background job (retry delay should have some randomness)
  • Invalid json from server == just fail

• Manual testing/understanding

  • Does Sentry catch crashes in PocketWorker? + rm TODO in PocketVideoFetchWorker
  • What happens when exceptions are thrown inside the PocketWorker? + rm TODO in PocketVideoFetchWorker
  • If a user's device is turned off and turned back on before the scheduled time, does WorkManager still run the job at the scheduled time?
    • If the device is turned off at the scheduled time and turned on later, does WorkManager still run the job (e.g. it catches up on old work)?
      • ^ File a bug for this (or maybe reopen the existing one?) Pocket fetch does not happen if device is off #2376

• Expose integration tests for QA - through custom intent flags

Impact

Some unused code needs to be removed and this background fetch functionality needs to be tested.

Acceptance criteria

All the above tasks are completed.

[nojunpark]

Just my 2 cents:

So this is my understanding of the background fetch works (which could be wrong):

• Background fetch is scheduled typically between anytime during 3AM and 6AM

• When the app is started, it checks for the fetched video JSON, and schedules the next fetch time

• Initially there is a hardcoded video JSON (so there is always something to show on the overlay)

• Assuming it will always fetch the list of video from pocket server that’s different from what we currently have

• We don’t have the record of the last successful fetch timestamp

If above is correct, it would be ideal if we have following grey-box tests that does the following, in addition to the usual unit tests. They cover pretty typical scenarios:

Checking whether update actually happens nominally:

• Opens app, checks the fetch is done, checks the next update schedule value is set.
• Change the next update schedule to currentTime + 1000ms
• wait to see whether pocketstore updates and refreshes overlay when reopened

Checking whether pocket updates past the specified time:

• Opens app, checks the fetch is done, checks the next update schedule value is set.
• Change the next update schedule to currentTime or past time (assuming this gets noticed by the app)
• Check whether the pocketstore updates (since the scheduled time is now in the past) and refreshes overlay when reopened

Checking whether app does not crash during network is down, and schedule reattempt

• Opens app, checks the fetch is done, checks the next update schedule value is set.
• Disable wifi
• Change the next update schedule to currentTime + 1000ms
• wait too see the app does not crash after failed fetch attempt, and check backoff time is properly set

Alternatively, above scenario can be done manually too, but then we need a way for the manual tester to view/edit next update schedule via about:config or so. Also, I think different locale test can be covered manually too. If we go that route, I'll update TestRail with additional test cases.

[mcomella]

@mcomella Yes! I updated the comment with my results of using throw RuntimeException

Keep in mind we don't get email updates for edits! :)

@dnarcese If that's the case, can you file a follow-up to investigate how we might get Sentry working in WorkManager?

[dnarcese]

Completed in b8b29d.