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

Added flag --silence-fork-pr-errors #885

Closed

Conversation

kinghrothgar
Copy link
Contributor

This adds flag to handle #884

@codecov
Copy link

codecov bot commented Dec 20, 2019

Codecov Report

Merging #885 into master will increase coverage by 0.02%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
+ Coverage   71.83%   71.85%   +0.02%     
==========================================
  Files          65       65              
  Lines        5247     5252       +5     
==========================================
+ Hits         3769     3774       +5     
  Misses       1191     1191              
  Partials      287      287
Impacted Files Coverage Δ
server/user_config.go 100% <ø> (ø) ⬆️
server/server.go 63.22% <100%> (+0.22%) ⬆️
cmd/server.go 78.87% <100%> (+0.09%) ⬆️
server/events/command_runner.go 50.63% <66.66%> (+0.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b066ff1...49cb12e. Read the comment docs.

@@ -68,6 +68,7 @@ const (
RepoWhitelistFlag = "repo-whitelist"
RequireApprovalFlag = "require-approval"
RequireMergeableFlag = "require-mergeable"
SilenceForkPRErrorsFlag = "silence-fork-pr-errors"
Copy link
Member

Choose a reason for hiding this comment

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

looks great! just need to update the tests in server_test.go look for each instance of the whitelist flag and then put your flag in there too

Copy link
Contributor Author

@kinghrothgar kinghrothgar Dec 20, 2019

Choose a reason for hiding this comment

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

Unless I'm missing it, it doesn't look like SilenceWhitelistErrorsFlag is tested in server_test.go. I can add tests for both Silence flags if you would like.

@kinghrothgar
Copy link
Contributor Author

Two questions about the tests (I am new to testing in go).

  1. When I run the tests of master (without my changes) there is one error: https://gist.github.com/kinghrothgar/0c4ffc7e3ef3f217fea5046308b8d31e. Is this expected?
  2. When I make this change to my PR'ed code:
diff --git a/server/events/command_runner_test.go b/server/events/command_runner_test.go
index 0f6d9b71..dc7ba0fc 100644
--- a/server/events/command_runner_test.go
+++ b/server/events/command_runner_test.go
@@ -157,7 +157,7 @@ func TestRunCommentCommand_ForkPRDisabled_SilenceEnabled(t *testing.T) {
 	t.Log("if a command is run on a forked pull request and forks are disabled and we are silencing errors do not comment with error")
 	vcsClient := setup(t)
 	ch.AllowForkPRs = false // by default it's false so don't need to reset
-	ch.SilenceForkPRErrors = true
+	ch.SilenceForkPRErrors = false
 	var pull github.PullRequest
 	modelPull := models.PullRequest{State: models.OpenPullState}
 	When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(&pull, nil)

to purposefully make my added test fail, it results in a panic: https://gist.github.com/kinghrothgar/b36386b70645e80326df6fe27e0bae0b. Is this expected or did I do something wrong?

@kinghrothgar kinghrothgar force-pushed the addSilenceForkPRErrorsFlag branch from 9d3791c to 49cb12e Compare December 20, 2019 15:53
@kinghrothgar
Copy link
Contributor Author

(Force push was just to get the go.mod and go.sum tiddy into it's own commit)

@kinghrothgar
Copy link
Contributor Author

@lkysow How does it look now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants