Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensures that the overdrive new titles script does not fail when cover… #2050

Merged
merged 2 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/palace/manager/core/coverage.py
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ def run_once(self, progress, count_as_covered=None):
transient_failures,
persistent_failures,
), results = self.process_batch_and_handle_results(batch_results)

self.finalize_batch()
# Update the running totals so that the service's eventual timestamp
# will have a useful .achievements.
progress.successes += successes
Expand Down Expand Up @@ -486,9 +486,6 @@ def process_batch_and_handle_results(self, batch):
num_ignored,
)

# Finalize this batch before moving on to the next one.
self.finalize_batch()

# For all purposes outside this method, treat an ignored identifier
# as a transient failure.
transient_failures += num_ignored
Expand Down Expand Up @@ -910,6 +907,7 @@ def run_on_specific_identifiers(self, identifiers):
while index < len(need_coverage):
batch = need_coverage[index : index + self.batch_size]
(s, t, p), r = self.process_batch_and_handle_results(batch)
self.finalize_batch()
successes += s
transient_failures += t
persistent_failures += p
Expand Down
13 changes: 7 additions & 6 deletions tests/manager/core/test_coverage.py
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,7 @@ def process_batch_and_handle_results(self, batch):
assert 1 == progress.successes
assert 2 == progress.transient_failures
assert 3 == progress.persistent_failures
assert provider.finalize_batch_called

assert (
"Items processed: 6. Successes: 1, transient failures: 2, persistent failures: 3"
Expand All @@ -535,9 +536,6 @@ def test_process_batch_and_handle_results(self, db: DatabaseTransactionFixture):
class MockProvider1(AlwaysSuccessfulCoverageProvider):
OPERATION = "i succeed"

def finalize_batch(self):
self.finalized = True

success_provider = MockProvider1(db.session)

batch = [i1, i2]
Expand All @@ -546,9 +544,6 @@ def finalize_batch(self):
# Two successes.
assert (2, 0, 0) == counts

# finalize_batch() was called.
assert True == success_provider.finalized

Comment on lines -549 to -551
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we would want to ensure that finalize_batch is called, since, in hindsight, it was easy to miss. Could you make a test that fails without the fixes you made above, but passes with them?

# Each represented with a CoverageRecord with status='success'
assert all(isinstance(x, CoverageRecord) for x in successes)
assert [CoverageRecord.SUCCESS] * 2 == [x.status for x in successes]
Expand Down Expand Up @@ -614,6 +609,9 @@ class MockProvider4(NeverSuccessfulCoverageProvider):
assert [CoverageRecord.PERSISTENT_FAILURE] * 2 == [x.status for x in results]
assert ["i will always fail"] * 2 == [x.operation for x in results]

assert not success_provider.finalize_batch_called
assert not persistent_failure_provider.finalize_batch_called

def test_process_batch(self, db: DatabaseTransactionFixture):
class Mock(BaseCoverageProvider):
SERVICE_NAME = "Some succeed, some fail."
Expand Down Expand Up @@ -1156,6 +1154,8 @@ def test_run_on_specific_identifiers(self, db: DatabaseTransactionFixture):
for i in not_to_be_tested:
assert i not in provider.attempts

assert provider.finalize_batch_called

def test_run_on_specific_identifiers_respects_cutoff_time(
self, db: DatabaseTransactionFixture
):
Expand Down Expand Up @@ -1199,6 +1199,7 @@ def test_run_on_specific_identifiers_respects_cutoff_time(
# reflect the failure.
assert records[0] == record
assert "What did you expect?" == record.exception
assert provider.finalize_batch_called

def test_run_never_successful(self, db: DatabaseTransactionFixture):
"""Verify that NeverSuccessfulCoverageProvider works the
Expand Down
4 changes: 4 additions & 0 deletions tests/mocks/mock.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,15 @@ class InstrumentedCoverageProvider(MockCoverageProvider, IdentifierCoverageProvi
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.attempts = []
self.finalize_batch_called = False

def process_item(self, item):
self.attempts.append(item)
return item

def finalize_batch(self):
self.finalize_batch_called = True


class InstrumentedWorkCoverageProvider(MockCoverageProvider, WorkCoverageProvider):
"""A WorkCoverageProvider that keeps track of every item it tried
Expand Down