diff --git a/src/github.rs b/src/github.rs index e873df6e..2c4ede7c 100644 --- a/src/github.rs +++ b/src/github.rs @@ -287,6 +287,7 @@ pub struct Label { /// and some don't. GitHub does include a few fields here, but they aren't /// needed at this time (merged_at, diff_url, html_url, patch_url, url). #[derive(Debug, serde::Deserialize)] +#[cfg_attr(test, derive(Default))] pub struct PullRequestDetails { /// This is a slot to hold the diff for a PR. /// diff --git a/src/handlers/check_commits.rs b/src/handlers/check_commits.rs index e4a2885b..24da8c0a 100644 --- a/src/handlers/check_commits.rs +++ b/src/handlers/check_commits.rs @@ -39,25 +39,42 @@ struct CheckCommitsState { last_labels: Vec, } -pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> anyhow::Result<()> { - let Event::Issue(event) = event else { - return Ok(()); - }; +fn should_handle_event(event: &IssuesEvent) -> bool { + // Reject non-PR + if !event.issue.is_pr() { + return false; + } + + // Reject rollups and draft pr + if event.issue.title.starts_with("Rollup of") || event.issue.draft { + return false; + } - let should_check = matches!( + // Check opened, reopened, synchronized and ready-for-review PRs + if matches!( event.action, IssuesAction::Opened | IssuesAction::Reopened | IssuesAction::Synchronize | IssuesAction::ReadyForReview - ) || event.has_base_changed(); + ) { + return true; + } - if !should_check || !event.issue.is_pr() { - return Ok(()); + // Also check PRs that changed their base branch (master -> beta) + if event.has_base_changed() { + return true; } - // Don't ping on rollups or draft PRs. - if event.issue.title.starts_with("Rollup of") || event.issue.draft { + false +} + +pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> anyhow::Result<()> { + let Event::Issue(event) = event else { + return Ok(()); + }; + + if !should_handle_event(event) { return Ok(()); } @@ -309,36 +326,37 @@ fn dummy_commit_from_body(sha: &str, body: &str) -> GithubCommit { } } -#[test] -#[rustfmt::skip] -fn test_warning_from_warnings() { - assert_eq!( - warning_from_warnings( - &[ +#[cfg(test)] +mod tests { + use super::*; + + #[test] + #[rustfmt::skip] + fn test_warning_from_warnings() { + assert_eq!( + warning_from_warnings( + &[ r#"This line should NOT be intend with 4 spaces, -but this line should!"# - .to_string() - ] - ), +but this line should!"#.to_string() + ] + ), r#":warning: **Warning** :warning: * This line should NOT be intend with 4 spaces, but this line should!"# - ); + ); - assert_eq!( - warning_from_warnings(&[ + assert_eq!( + warning_from_warnings(&[ r#"This is warning 1. Look at this list: - 12 - - 13"# - .to_string(), + - 13"#.to_string(), r#"This is warning 2. - 123456789 -"# - .to_string() - ]), +"#.to_string() + ]), r#":warning: **Warning** :warning: * This is warning 1. @@ -348,5 +366,141 @@ r#":warning: **Warning** :warning: - 13 * This is warning 2. - 123456789"# - ); + ); + } + + fn make_opened_pr_event() -> IssuesEvent { + IssuesEvent { + action: IssuesAction::Opened, + issue: crate::github::Issue { + number: 123, + body: "My PR body".to_string(), + created_at: Default::default(), + updated_at: Default::default(), + merge_commit_sha: Default::default(), + title: "Some title".to_string(), + html_url: Default::default(), + user: crate::github::User { + login: "user".to_string(), + id: 654123, + }, + labels: Default::default(), + assignees: Default::default(), + pull_request: Some(Default::default()), + merged: false, + draft: false, + comments: Default::default(), + comments_url: Default::default(), + repository: Default::default(), + base: Some(crate::github::CommitBase { + sha: "fake-sha".to_string(), + git_ref: "master".to_string(), + repo: None, + }), + head: Some(crate::github::CommitBase { + sha: "fake-sha".to_string(), + git_ref: "master".to_string(), + repo: None, + }), + state: crate::github::IssueState::Open, + milestone: None, + mergeable: None, + author_association: octocrab::models::AuthorAssociation::Contributor, + }, + changes: None, + repository: crate::github::Repository { + full_name: "rust-lang/rust".to_string(), + default_branch: "master".to_string(), + fork: false, + parent: None, + }, + sender: crate::github::User { + login: "rustbot".to_string(), + id: 987654, + }, + } + } + + #[test] + fn test_pr_closed() { + let mut event = make_opened_pr_event(); + event.action = IssuesAction::Closed; + assert!(!should_handle_event(&event)); + } + + #[test] + fn test_pr_opened() { + let event = make_opened_pr_event(); + assert!(should_handle_event(&event)); + } + + #[test] + fn test_not_pr() { + let mut event = make_opened_pr_event(); + event.issue.pull_request = None; + assert!(!should_handle_event(&event)); + } + + #[test] + fn test_pr_rollup() { + let mut event = make_opened_pr_event(); + event.issue.title = "Rollup of 6 pull requests".to_string(); + assert!(!should_handle_event(&event)); + } + + #[test] + fn test_pr_draft() { + let mut event = make_opened_pr_event(); + event.issue.draft = true; + assert!(!should_handle_event(&event)); + } + + #[test] + fn test_pr_ready() { + let mut event = make_opened_pr_event(); + event.action = IssuesAction::ReadyForReview; + assert!(should_handle_event(&event)); + } + + #[test] + fn test_pr_reopened() { + let mut event = make_opened_pr_event(); + event.action = IssuesAction::Reopened; + assert!(should_handle_event(&event)); + } + + #[test] + fn test_pr_synchronized() { + let mut event = make_opened_pr_event(); + event.action = IssuesAction::Synchronize; + assert!(should_handle_event(&event)); + } + + #[test] + fn test_pr_edited_title() { + let mut event = make_opened_pr_event(); + event.action = IssuesAction::Edited; + event.changes = Some(crate::github::Changes { + base: None, + body: None, + title: Some(crate::github::ChangeInner { + from: "Previous title".to_string(), + }), + }); + assert!(!should_handle_event(&event)); + } + + #[test] + fn test_pr_edited_base() { + let mut event = make_opened_pr_event(); + event.action = IssuesAction::Edited; + event.changes = Some(crate::github::Changes { + title: None, + body: None, + base: Some(crate::github::ChangeInner { + from: "master".to_string(), + }), + }); + assert!(should_handle_event(&event)); + } }