Skip to content
Open
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
101 changes: 101 additions & 0 deletions src/lando/api/tests/test_landings.py
Original file line number Diff line number Diff line change
Expand Up @@ -1381,6 +1381,107 @@ def _make_handover_job(target_repo: Repo, handover_repo: Repo) -> LandingJob:
return _make_handover_job


@pytest.mark.django_db
def test_deferred_jobs(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

test_deferred_jobs is not very descriptive. How about test_closed_tree_defers_jobs_without_blocking_other_repos?

repo_mc: Callable,
treestatusdouble: TreeStatusDouble,
get_landing_worker: Callable,
make_landing_job,
create_patch_revision,
normal_patch,
mock_phab_trigger_repo_update_apply_async: mock.Mock,
):
# This test simulates a number of jobs in two different repos
# being submitted, and ensures that when closing a tree the
# queue is not blocked by the jobs in the queue that target
# the closed tree.
Comment on lines +1394 to +1397
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: this could be a docstring comment.


def refresh(jobs):
[job.refresh_from_db() for job in jobs]

git_repo_1 = repo_mc(SCMType.GIT, hooks_enabled=False, name="repo1")
git_repo_2 = repo_mc(SCMType.GIT, hooks_enabled=False, name="repo2")
git_worker = get_landing_worker(SCMType.GIT)
git_worker.worker_instance.applicable_repos.add(git_repo_1)
git_worker.worker_instance.applicable_repos.add(git_repo_2)

treestatusdouble.open_tree(git_repo_1.name)
treestatusdouble.open_tree(git_repo_2.name)
git_worker.refresh_active_repos()

job_params = {
"status": JobStatus.SUBMITTED,
"requester_email": "test@example.com",
}

job_params["target_repo"] = git_repo_1
jobs = []
for i in (1, 2, 3):
jobs.append(
make_landing_job(
revisions=[
create_patch_revision(1, patch=normal_patch(3), revision_id=i)
],
**job_params,
Comment on lines +1417 to +1425
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We might as well pass target_repo directly, without using job_params?

Suggested change
job_params["target_repo"] = git_repo_1
jobs = []
for i in (1, 2, 3):
jobs.append(
make_landing_job(
revisions=[
create_patch_revision(1, patch=normal_patch(3), revision_id=i)
],
**job_params,
jobs = []
for i in (1, 2, 3):
jobs.append(
make_landing_job(
revisions=[
create_patch_revision(1, patch=normal_patch(3), revision_id=i)
],
target_repo=git_repo_1,

)
)

job_params["target_repo"] = git_repo_2
for i in (4, 5, 6):
jobs.append(
make_landing_job(
revisions=[
create_patch_revision(1, patch=normal_patch(3), revision_id=i)
],
**job_params,
Comment on lines +1429 to +1436
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto

Suggested change
job_params["target_repo"] = git_repo_2
for i in (4, 5, 6):
jobs.append(
make_landing_job(
revisions=[
create_patch_revision(1, patch=normal_patch(3), revision_id=i)
],
**job_params,
for i in (4, 5, 6):
jobs.append(
make_landing_job(
revisions=[
create_patch_revision(1, patch=normal_patch(3), revision_id=i)
],
target_repo: git_repo_2,

)
)

assert LandingJob.objects.all().count() == 6
assert all(
job.status == JobStatus.SUBMITTED for job in LandingJob.objects.all()
), "All jobs should be in a submitted state."

git_worker.start(max_loops=1)
refresh(jobs)

assert jobs[0].status == JobStatus.LANDED, "First job should have landed."

# Close first tree.
treestatusdouble.close_tree(git_repo_1.name)
git_worker.start(max_loops=1)

# Second job should be deferred now, since we did not refresh active repos.
# All remaining jobs should be submitted. Active repos are refreshed when
# the last job did not complete.
refresh(jobs)
assert jobs[1].status == JobStatus.DEFERRED, "Second job should be deferred."
assert all(
job.status == JobStatus.SUBMITTED for job in jobs[2:]
), "Remaining jobs should be submitted."

git_worker.start(max_loops=10)
refresh(jobs)
assert all(
job.status == JobStatus.LANDED for job in jobs[3:]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Checking the job statuses by slicing the jobs list is brittle. If someone needs to extend the test to include another job they'll have to tweak these slicing values based on the insertion order into the main jobs list.

We should instead create a separate list of repo1/repo2 jobs and check the behaviour in those lists instead:

repo1_jobs = [job for job in jobs if job.target_repo == git_repo_1]
repo2_jobs = [job for job in jobs if job.target_repo == git_repo_2]

assert all(
    job.status == JobStatus.LANDED for job in repo2_jobs
), "All repo2 jobs should be landed."

assert repo1_jobs[0].status == JobStatus.DEFERRED, "First remaining repo1 job should be deferred."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agree.

), "Remaining repo2 jobs should be landed."

refresh(jobs)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need to refresh here, as we haven't changed anything since the previous refresh.

assert (
jobs[1].status == JobStatus.DEFERRED
), "Second repo1 job should still be deferred."
assert (
jobs[2].status == JobStatus.SUBMITTED
), "Third repo1 job should still be submitted."

treestatusdouble.open_tree(git_repo_1.name)
git_worker.start(max_loops=5)
refresh(jobs)
assert all(
job.status == JobStatus.LANDED for job in jobs
), "All jobs should have landed."


@pytest.mark.parametrize(
"closed_repos,",
[
Expand Down
14 changes: 14 additions & 0 deletions src/lando/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,17 @@
+TEST
""".lstrip()

PATCH_NORMAL_4 = r"""
# HG changeset patch
# User Test User <test@example.com>
# Date 0 0
# Thu Jan 01 00:00:00 1970 +0000
# Diff Start Line 7
no bug: patch that applies over and over
diff --git a/{filename} b/{filename}
new file mode 100644
""".lstrip()

PATCH_GIT_1 = """\
From dd187015cd85d59c2a65a3a18c67b2b05e7739b9 Mon Sep 17 00:00:00 2001
From: Py Test <pytest@lando.example.net>
Expand Down Expand Up @@ -213,9 +224,12 @@ def normal_patch():
PATCH_NORMAL_1,
PATCH_NORMAL_2,
PATCH_NORMAL_3,
PATCH_NORMAL_4,
]

def _patch(number=0):
if number == 3:
return PATCH_NORMAL_4.format(filename=uuid.uuid4())
Comment on lines +231 to +232
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this new infinitely-applying patch is a special case, I think it would be better to create a new success_patch or something similar, instead of shoehorning it into normal_patch only when the input argument is 3.

If there's some reason this should be kept in normal_patch, we should write a comment explaining why, and find a better way to pick this patch instead of making 3 a magic-number.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agree.

return _patches[number]

return _patch
Expand Down
Loading