Comment on source PR if backport fails.

[rcorre]

Octobot reports a backport failure via a DM to the PR author, which can be missed and lost, leaving no persistent record of what happened.

Octobot should additionally comment on the PR itself.

I haven't tested this live, nor via unit tests :grimacing:.

For the former, I tried to create an org in public github but got a bit stumped on how to create an app/get a token.

For the latter, it seemed like I needed to create a full Runner in pr_merge_test, which hasn't been done before. The runner takes a GithubSessionFactory. I tried to create a Mock for that, but since new_session returns Result<GithubSession> (not Result<Session>), I couldn't have my mock return a MockSession instead. Here's what I tried:

diff --git a/tests/mocks/mock_github.rs b/tests/mocks/mock_github.rs
index 8f4f549..17d7fbd 100644
--- a/tests/mocks/mock_github.rs
+++ b/tests/mocks/mock_github.rs
@@ -3,7 +3,28 @@ use std::thread;

 use octobot::errors::*;
 use octobot::github::*;
-use octobot::github::api::Session;
+use octobot::github::api::{Session,GithubSession,GithubSessionFactory};
+use mocks::mock_github::MockGithub;
+
+pub struct MockGithubSessionFactory {}
+
+impl GithubSessionFactory for MockGithubSessionFactory {
+    fn bot_name(&self) -> String {
+        "MockBotName".to_string()
+    }
+
+    fn get_token_org(&self, org: &str) -> Result<String> {
+        Ok("MockTokenOrg".to_string())
+    }
+
+    fn get_token_repo(&self, owner: &str, repo: &str) -> Result<String> {
+        Ok("MockTokenRepo".to_string())
+    }
+
+    fn new_session(&self, owner: &str, repo: &str) -> Result<GithubSession> {
+        Ok(MockGithub::new())
+    }
+}

 pub struct MockGithub {
     host: String,
diff --git a/tests/pr_merge_test.rs b/tests/pr_merge_test.rs
index 988f708..5e55b85 100644
--- a/tests/pr_merge_test.rs
+++ b/tests/pr_merge_test.rs
@@ -1,10 +1,17 @@
 mod git_helper;
 mod mocks;

+use std::sync::Arc;
 use git_helper::temp_git::TempGit;
 use mocks::mock_github::MockGithub;
+use mocks::mock_github::MockGithubSessionFactory;
 use octobot::github;
 use octobot::pr_merge;
+use octobot::config::Config;
+use octobot::db::Database;
+use octobot::git_clone_manager::GitCloneManager;
+use mocks::mock_slack::MockSlack;
+use tempdir::TempDir;

 #[test]
 fn test_pr_merge_basic() {
@@ -332,3 +339,22 @@ fn test_pr_merge_conventional_commit() {
     assert_eq!(456, created_pr.number);
     assert_eq!("", git.run_git(&["diff", "master", "origin/my-feature-branch-1.0"]));
 }
+
+#[test]
+fn test_pr_merge_backport_failure() {
+    let git = TempGit::new();
+
+    let temp_dir = TempDir::new("repos.rs").unwrap();
+    let db_file = temp_dir.path().join("db.sqlite3");
+    let db = Database::new(&db_file.to_string_lossy()).expect("create temp database");
+    let config = Arc::new(Config::new(db));
+    let github = Arc::new(MockGithubSessionFactory::new());
+    //let clone_mgr = Arc::new(GitCloneManager::new(github, config));
+    let slack = MockSlack::new(vec![]);
+    let runner = pr_merge::new_runner(
+        config,
+        github,
+        //clone_mgr,
+        slack.new_sender(),
+    );
+}

Which errors expected struct GithubSession, found struct MockGithub. I don't know enough about Rust to figure this out, but I might try again later. new_session returning a Box could work, perhaps? Or I'm on totally the wrong track trying to test this.

[matthauck]

Nice! Looking a bit at it now, it is definitely not setup to test the runner / cloning part. I think that's why merge_pull_request was separated out as a public function there -- to be able to test that functionality.

Thought 1: Maybe we should just move the slack messaging into merge_pull_request? Not sure there's really a good reason for it to be inside the runner instead.

Thought 2: It would be cool to also add a label. Maybe failed-backport or something? That would make these PRs easily searchable.

[rcorre]

Excuse the force push, it was pretty much a rewrite. It's tested now though!

[rcorre]

retitle the PR to indicate this is a stepping stone

ah, oops, it was a pretty simple addition so I just pushed another commit before I noticed this.

FWIW, with this rewrite I think we could make merge_pull_request private and just exercise the worker, if that's better. Though I guess that would make the test async.

[rcorre]

reopened as #277 to get build running