refactor: simplify/extract logic to assert GC review due dates

The following discussion from !1100 (merged) should be addressed:

  • @jaime started a discussion:

    minor nit: we use defaultReviewAfterDelay+minReviewAfterJitter and defaultReviewAfterDelay+ maxReviewAfterJitter in many places so they could be defined as constants at the top of this file.

    We also do a bunch of assertions repeatedly so they could be extracted into an assertGCReviews function for example. This can probably be done later as a nice-to-have 😄

Implementation Guide

The registry/datastore/gc_integration_test.go test file currently contains several instances of the following assertions:

require.Greater(t, rr[0].ReviewAfter, b.CreatedAt.Add(defaultReviewAfterDelay+minReviewAfterJitter))
require.Less(t, rr[0].ReviewAfter, b.CreatedAt.Add(defaultReviewAfterDelay+maxReviewAfterJitter))

These assertions test that the review delay for the review queue object falls within the expected time. We use a jitter to reduce contention over the review queue since it's possible for many reviews to be queued simultaneously, and since multiple instances of manifest or blob workers may be attempting to process queue items concurrently.

We can simplify these statements in two ways, first by consolidating defaultReviewAfterDelay+minReviewAfterJitter and defaultReviewAfterDelay+maxReviewAfterJitter into constants representing the min and max expected ReviewAfterDelay respectively. Secondly, we can use require.WithinRange to replace the require.Greater and require.Less calls with a single assertion.

Testing

The following command can be used to test locally:

go test -v -tags=integration -failfast  github.com/docker/distribution/registry/datastore

The integration tag is conventionally used in this project to enable tests that require a connection to a database. Setting up a local Postgres 12 database can speed up test cycles compared to CI jobs. The tests will automatically handle the migrations and setup/teardown operations they require to run.

These tests use several environment variables to connect to the database. See the .gitlab-ci.yml for these variables, all named starting with REGISTRY_DATABASE_. You'll need to set these variables so that they correspond to the database you intend the tests to connect to.

Edited by SAhmed