Skip to content

Commit b58d6ac

Browse files
Sakib25800Sakib25800
committed
Add merge queue ordering tests
1 parent 5a3ef9c commit b58d6ac

File tree

5 files changed

+208
-40
lines changed

5 files changed

+208
-40
lines changed

‎src/bors/handlers/pr_events.rs

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -659,18 +659,22 @@ mod tests {
659659
#[sqlx::test]
660660
async fn enqueue_prs_on_push_to_branch(pool: sqlx::PgPool) {
661661
run_test(pool, async |tester| {
662-
tester.open_pr(default_repo_name(), false).await?;
662+
let pr = tester.open_pr(default_repo_name(), false).await?;
663663
tester.push_to_branch(default_branch_name()).await?;
664664
tester
665-
.wait_for_default_pr(|pr| pr.mergeable_state == MergeableState::Unknown)
665+
.wait_for_pr(default_repo_name(), pr.number.0, |pr| {
666+
pr.mergeable_state == MergeableState::Unknown
667+
})
666668
.await?;
667669
tester
668670
.default_repo()
669671
.lock()
670-
.get_pr_mut(default_pr_number())
672+
.get_pr_mut(pr.number.0)
671673
.mergeable_state = OctocrabMergeableState::Dirty;
672674
tester
673-
.wait_for_default_pr(|pr| pr.mergeable_state == MergeableState::HasConflicts)
675+
.wait_for_pr(default_repo_name(), pr.number.0, |pr| {
676+
pr.mergeable_state == MergeableState::HasConflicts
677+
})
674678
.await?;
675679
Ok(())
676680
})
@@ -680,17 +684,21 @@ mod tests {
680684
#[sqlx::test]
681685
async fn enqueue_prs_on_pr_opened(pool: sqlx::PgPool) {
682686
run_test(pool, async |tester| {
683-
tester.open_pr(default_repo_name(), false).await?;
687+
let pr = tester.open_pr(default_repo_name(), false).await?;
684688
tester
685-
.wait_for_default_pr(|pr| pr.mergeable_state == MergeableState::Unknown)
689+
.wait_for_pr(default_repo_name(), pr.number.0, |pr| {
690+
pr.mergeable_state == MergeableState::Unknown
691+
})
686692
.await?;
687693
tester
688694
.default_repo()
689695
.lock()
690-
.get_pr_mut(default_pr_number())
696+
.get_pr_mut(pr.number.0)
691697
.mergeable_state = OctocrabMergeableState::Dirty;
692698
tester
693-
.wait_for_default_pr(|pr| pr.mergeable_state == MergeableState::HasConflicts)
699+
.wait_for_pr(default_repo_name(), pr.number.0, |pr| {
700+
pr.mergeable_state == MergeableState::HasConflicts
701+
})
694702
.await?;
695703
Ok(())
696704
})

‎src/bors/merge_queue.rs

Lines changed: 128 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ mod tests {
359359
github::CommitSha,
360360
tests::{
361361
BorsTester,
362-
mocks::{BorsBuilder, GitHubState, WorkflowEvent, default_repo_name},
362+
mocks::{BorsBuilder, Comment, GitHubState, WorkflowEvent, default_repo_name},
363363
},
364364
};
365365

@@ -582,4 +582,131 @@ mod tests {
582582
&["merge-main-sha1-pr-1-sha-0"],
583583
);
584584
}
585+
586+
#[sqlx::test]
587+
async fn merge_queue_sequential_order(pool: sqlx::PgPool) {
588+
let gh = run_merge_queue_test(pool, async |tester| {
589+
tester.post_comment("@bors r+").await?;
590+
tester.expect_comments(1).await;
591+
592+
let pr2 = tester.open_pr(default_repo_name(), false).await?;
593+
let pr3 = tester.open_pr(default_repo_name(), false).await?;
594+
595+
tester
596+
.post_comment(Comment::new(default_repo_name(), pr2.number.0, "@bors r+"))
597+
.await?;
598+
tester
599+
.post_comment(Comment::new(default_repo_name(), pr3.number.0, "@bors r+"))
600+
.await?;
601+
602+
tester
603+
.expect_comment_on_pr(default_repo_name(), pr2.number.0)
604+
.await?;
605+
tester
606+
.expect_comment_on_pr(default_repo_name(), pr3.number.0)
607+
.await?;
608+
609+
tester.process_merge_queue().await;
610+
tester.expect_comments(1).await;
611+
tester.workflow_full_success(tester.auto_branch()).await?;
612+
tester.expect_comments(1).await;
613+
614+
tester.process_merge_queue().await;
615+
tester
616+
.expect_comment_on_pr(default_repo_name(), pr2.number.0)
617+
.await?;
618+
tester.workflow_full_success(tester.auto_branch()).await?;
619+
tester
620+
.expect_comment_on_pr(default_repo_name(), pr2.number.0)
621+
.await?;
622+
623+
tester.process_merge_queue().await;
624+
tester
625+
.expect_comment_on_pr(default_repo_name(), pr3.number.0)
626+
.await?;
627+
tester.workflow_full_success(tester.auto_branch()).await?;
628+
tester
629+
.expect_comment_on_pr(default_repo_name(), pr3.number.0)
630+
.await?;
631+
632+
Ok(())
633+
})
634+
.await;
635+
636+
gh.check_sha_history(
637+
default_repo_name(),
638+
"main",
639+
&[
640+
"main-sha1",
641+
"merge-main-sha1-pr-1-sha-0",
642+
"merge-merge-main-sha1-pr-1-sha-0-pr-2-sha-1",
643+
"merge-merge-merge-main-sha1-pr-1-sha-0-pr-2-sha-1-pr-3-sha-2",
644+
],
645+
);
646+
}
647+
648+
#[sqlx::test]
649+
async fn merge_queue_priority_order(pool: sqlx::PgPool) {
650+
let gh = run_merge_queue_test(pool, async |tester| {
651+
let pr2 = tester.open_pr(default_repo_name(), false).await?;
652+
let pr3 = tester.open_pr(default_repo_name(), false).await?;
653+
654+
tester.post_comment("@bors r+").await?;
655+
tester
656+
.post_comment(Comment::new(default_repo_name(), pr2.number.0, "@bors r+"))
657+
.await?;
658+
tester
659+
.post_comment(Comment::new(
660+
default_repo_name(),
661+
pr3.number.0,
662+
"@bors r+ p=3",
663+
))
664+
.await?;
665+
666+
tester.expect_comments(1).await;
667+
tester
668+
.expect_comment_on_pr(default_repo_name(), pr2.number.0)
669+
.await?;
670+
tester
671+
.expect_comment_on_pr(default_repo_name(), pr3.number.0)
672+
.await?;
673+
674+
tester.process_merge_queue().await;
675+
tester.expect_comments(1).await;
676+
tester.workflow_full_success(tester.auto_branch()).await?;
677+
tester.expect_comments(1).await;
678+
679+
tester.process_merge_queue().await;
680+
tester
681+
.expect_comment_on_pr(default_repo_name(), pr3.number.0)
682+
.await?;
683+
tester.workflow_full_success(tester.auto_branch()).await?;
684+
tester
685+
.expect_comment_on_pr(default_repo_name(), pr3.number.0)
686+
.await?;
687+
688+
tester.process_merge_queue().await;
689+
tester
690+
.expect_comment_on_pr(default_repo_name(), pr2.number.0)
691+
.await?;
692+
tester.workflow_full_success(tester.auto_branch()).await?;
693+
tester
694+
.expect_comment_on_pr(default_repo_name(), pr2.number.0)
695+
.await?;
696+
697+
Ok(())
698+
})
699+
.await;
700+
701+
gh.check_sha_history(
702+
default_repo_name(),
703+
"main",
704+
&[
705+
"main-sha1",
706+
"merge-main-sha1-pr-1-sha-0",
707+
"merge-merge-main-sha1-pr-1-sha-0-pr-3-sha-1",
708+
"merge-merge-merge-main-sha1-pr-1-sha-0-pr-3-sha-1-pr-2-sha-2",
709+
],
710+
);
711+
}
585712
}

‎src/tests/mocks/bors.rs

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,20 @@ impl BorsTester {
318318
.content)
319319
}
320320

321+
/// Wait until the next bot comment is received on the specified PR and consume it.
322+
pub async fn expect_comment_on_pr(
323+
&mut self,
324+
repo: GithubRepoName,
325+
pr: u64,
326+
) -> anyhow::Result<String> {
327+
Ok(self
328+
.http_mock
329+
.gh_server
330+
.get_comment(repo, pr)
331+
.await?
332+
.content)
333+
}
334+
321335
//-- Generation of GitHub events --//
322336
pub async fn post_comment<C: Into<Comment>>(&mut self, comment: C) -> anyhow::Result<()> {
323337
self.webhook_comment(comment.into()).await
@@ -460,10 +474,23 @@ impl BorsTester {
460474
let number = {
461475
let repo = self.github.get_repo(&repo_name);
462476
let repo = repo.lock();
463-
repo.pull_requests.keys().max().copied().unwrap_or(1)
477+
repo.pull_requests.keys().max().copied().unwrap_or(0) + 1
464478
};
465479

466-
let pr = PullRequest::new(repo_name, number, User::default_pr_author(), is_draft);
480+
let pr = PullRequest::new(
481+
repo_name.clone(),
482+
number,
483+
User::default_pr_author(),
484+
is_draft,
485+
);
486+
487+
// Add the PR to the repository
488+
{
489+
let repo = self.github.get_repo(&repo_name);
490+
let mut repo = repo.lock();
491+
repo.pull_requests.insert(number, pr.clone());
492+
}
493+
467494
self.send_webhook(
468495
"pull_request",
469496
GitHubPullRequestEventPayload::new(pr.clone(), "opened", None),

‎src/tests/mocks/pull_request.rs

Lines changed: 33 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -52,41 +52,44 @@ pub async fn mock_pull_requests(
5252
.mount(mock_server)
5353
.await;
5454

55+
let repo_clone = repo.clone();
56+
dynamic_mock_req(
57+
move |_req: &Request, [pr_number]: [&str; 1]| {
58+
let pr_number: u64 = pr_number.parse().unwrap();
59+
let pull_request_error = repo_clone.lock().pull_request_error;
60+
if pull_request_error {
61+
ResponseTemplate::new(500)
62+
} else if let Some(pr) = repo_clone.lock().pull_requests.get(&pr_number) {
63+
ResponseTemplate::new(200).set_body_json(GitHubPullRequest::from(pr.clone()))
64+
} else {
65+
ResponseTemplate::new(404)
66+
}
67+
},
68+
"GET",
69+
format!("^/repos/{repo_name}/pulls/([0-9]+)$"),
70+
)
71+
.mount(mock_server)
72+
.await;
73+
74+
mock_pr_comments(repo.clone(), comments_tx.clone(), mock_server).await;
75+
5576
let prs = repo.lock().pull_requests.clone();
5677
for &pr_number in prs.keys() {
57-
let repo_clone = repo.clone();
58-
Mock::given(method("GET"))
59-
.and(path(format!("/repos/{repo_name}/pulls/{pr_number}")))
60-
.respond_with(move |_: &Request| {
61-
let pull_request_error = repo_clone.lock().pull_request_error;
62-
if pull_request_error {
63-
ResponseTemplate::new(500)
64-
} else if let Some(pr) = repo_clone.lock().pull_requests.get(&pr_number) {
65-
ResponseTemplate::new(200).set_body_json(GitHubPullRequest::from(pr.clone()))
66-
} else {
67-
ResponseTemplate::new(404)
68-
}
69-
})
70-
.mount(mock_server)
71-
.await;
72-
73-
mock_pr_comments(repo.clone(), pr_number, comments_tx.clone(), mock_server).await;
7478
mock_pr_labels(repo.clone(), repo_name.clone(), pr_number, mock_server).await;
7579
}
7680
}
7781

7882
async fn mock_pr_comments(
7983
repo: Arc<Mutex<Repo>>,
80-
pr_number: u64,
8184
comments_tx: Sender<Comment>,
8285
mock_server: &MockServer,
8386
) {
8487
let repo_name = repo.lock().name.clone();
85-
Mock::given(method("POST"))
86-
.and(path(format!(
87-
"/repos/{repo_name}/issues/{pr_number}/comments",
88-
)))
89-
.respond_with(move |req: &Request| {
88+
let repo_name_clone = repo_name.clone();
89+
dynamic_mock_req(
90+
move |req: &Request, [pr_number]: [&str; 1]| {
91+
let pr_number: u64 = pr_number.parse().unwrap();
92+
9093
#[derive(Deserialize)]
9194
struct CommentCreatePayload {
9295
body: String,
@@ -97,7 +100,7 @@ async fn mock_pr_comments(
97100
let pr = repo.pull_requests.get_mut(&pr_number).unwrap();
98101
let comment_id = pr.next_comment_id();
99102

100-
let comment = Comment::new(repo_name.clone(), pr_number, &comment_payload.body)
103+
let comment = Comment::new(repo_name_clone.clone(), pr_number, &comment_payload.body)
101104
.with_author(User::bors_bot())
102105
.with_id(comment_id);
103106

@@ -106,9 +109,12 @@ async fn mock_pr_comments(
106109
// `tx.send()`.
107110
comments_tx.try_send(comment.clone()).unwrap();
108111
ResponseTemplate::new(201).set_body_json(GitHubComment::from(comment))
109-
})
110-
.mount(mock_server)
111-
.await;
112+
},
113+
"POST",
114+
format!("^/repos/{repo_name}/issues/([0-9]+)/comments$"),
115+
)
116+
.mount(mock_server)
117+
.await;
112118
}
113119

114120
async fn mock_pr_labels(

‎src/utils/sort_queue.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,8 @@ fn get_queue_blocking_priority(pr: &PullRequestModel) -> u32 {
4646
fn get_status_priority(pr: &PullRequestModel) -> u32 {
4747
match &pr.auto_build {
4848
Some(build) => match build.status {
49-
BuildStatus::Success => 1,
50-
BuildStatus::Pending => 1, // Same as success since queue blocking is handled separately
49+
BuildStatus::Success => 0,
50+
BuildStatus::Pending => 1,
5151
BuildStatus::Failure => 3,
5252
BuildStatus::Cancelled | BuildStatus::Timeouted => 2,
5353
},

0 commit comments

Comments
 (0)