Update integration tests for new branch protection API. by varadarajana · Pull Request #509 · google/go-github

varadarajana · 2017-01-08T09:57:34Z

Integration tests update branches were broken since old structures were used. This has been modified. Integration folder requires ProtectionRequest structure to be available by RepositoryService. Protection also has been changed.

Resolves #507.
Updates #310.

googlebot · 2017-01-08T09:57:36Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

varadarajana · 2017-01-08T10:10:08Z

I signed it!

googlebot · 2017-01-08T10:10:10Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

varadarajana · 2017-01-08T10:11:57Z

I signed it!

dmitshur · 2017-01-08T18:34:29Z

tests/integration/activity_test.go

 	"testing"

-	"github.com/google/go-github/github"
+	"go-github/github"


This change shouldn't be done. Here, and below.

dmitshur · 2017-01-08T21:46:55Z

github/repos.go

+		},
+	}
+	return input
+}


This new method isn't needed. Everything it does can and should be done inside TestRepositories_EditBranches where it's currently being called.

Yes, I agree. I have made these changes and also committed them. Are you able to see these changes. Am i supposed to create a new PR?

You shouldn't need to make a new PR.

After you've committed the changes, make sure to push them to the remote master branch of your fork https://github.com/varadarajana/go-github. That's the branch this pull request is based on.

After that, this PR will include your latest commits.

See https://help.github.com/articles/pushing-to-a-remote/ for additional information.

dmitshur · 2017-01-08T21:50:32Z

tests/integration/repos_test.go

+	protectionRequest.RequiredStatusChecks.Strict = github.Bool(true)
+	protectionRequest.RequiredStatusChecks.Contexts = &[]string{"continuous-integration"}
+	protectionRequest.Restrictions.Users = &[]string{"u"}
+	protectionRequest.Restrictions.Teams = &[]string{"t"}


It'll look something like this:

protectionRequest := &github.ProtectionRequest{ RequiredStatusChecks: &github.RequiredStatusChecks{ IncludeAdmins: github.Bool(true), Strict: github.Bool(true), Contexts: &[]string{"continuous-integration"}, }, Restrictions: &github.BranchRestrictionsRequest{ Users: &[]string{"u"}, Teams: &[]string{"t"}, }, }

varadarajana · 2017-01-09T16:08:53Z

I have made all the changes suggested by @shurcooL, somehow my CI builds are failing. If i look at logs i do not see any issue.
My local builds work.

dmitshur · 2017-01-09T16:42:36Z

This is getting closer.

The CI logs do state the problem. Look at https://travis-ci.org/google/go-github/jobs/190304467, it says in red text near the top:

The command "diff -u <(echo -n) <(gofmt -d -s .)" exited with 1.

That means some of the files are not correctly formattend with gofmt.

You'll want to run gofmt on the problematic files to fix that. You can find documentation for that command at https://golang.org/cmd/gofmt/, https://golang.org/cmd/go/#hdr-Run_gofmt_on_package_sources, and go help fmt.

Edit: I see you've done exactly that while I was typing this. 👍

varadarajana · 2017-01-09T16:45:14Z

@shurcooL thank you.

Is the code review approved? Now do I need to close and comment?

dmitshur · 2017-01-09T16:45:26Z

tests/integration/repos_test.go

-	}
+	//if *protection.RequiredStatusChecks.IncludeAdmins != "everyone" {
+	//	t.Fatalf("RequiredStatusChecks should be enabled for everyone, set for: %v", *protection.RequiredStatusChecks.EnforcementLevel)
+	//}


Why is this code commented out?

IncludeAdmins is a bool, and this is checked in the code before as i set it to true.

if !*protection.RequiredStatusChecks.IncludeAdmins { t.Fatalf("Branch %v of repo %v should be protected, but is not!", "master", *repo.Name) }

Got it. In that case, you should remove the commented out code, since it's not needed anymore.

sure i will do that now and submit.

varadarajana · 2017-01-09T17:03:22Z

@shurcooL I have removed the commented lines now.

dmitshur · 2017-01-09T18:38:17Z

tests/integration/repos_test.go

+
+	protection, _, err := client.Repositories.UpdateBranchProtection("o", "r", "b", input)
 	if err != nil {
 		t.Fatalf("Repositories.EditBranch() returned error: %v", err)


This error message is out of date. It refers to EditBranch which you've replaced by UpdateBranchProtection.

corrected this and the checked the other function names too.

dmitshur · 2017-01-09T18:39:14Z

tests/integration/repos_test.go


-	if !*branch.Protection.Enabled {
+	if !*protection.RequiredStatusChecks.IncludeAdmins {
 		t.Fatalf("Branch %v of repo %v should be protected, but is not!", "master", *repo.Name)


The error message here for if !*protection.RequiredStatusChecks.IncludeAdmins { does not align, it should be something like t.Fatalf("RequiredStatusChecks should be enabled for everyone, set for: %v", ...).

See inside of the if *branch.Protection.RequiredStatusChecks.EnforcementLevel != "everyone" { block you've deleted below.

At the same time, is there anything that checks that branch is protected? Perhaps you should have a check like this:

if !*protection.Restrictions != ... { t.Fatalf("Branch %v of repo %v should be protected, but is not!", "master", *repo.Name) }

I'm not exactly sure.

I have added test for methods also.

dmitshur · 2017-01-09T18:44:05Z

tests/integration/repos_test.go

-		Contexts:         &[]string{"continous-integration"},
+	branch.Protected = github.Bool(true)
+
+	input := &github.ProtectionRequest{


input is not a very descriptive name. Can you call it pr, protectionRequest, or simply inline it with the call to UpdateBranchProtection?

I have updated this to pr and will be available in the next commit.

dmitshur

This is closer now. Some inline questions/comments.

dmitshur · 2017-01-10T18:24:24Z

tests/integration/repos_test.go

-	branch.Protection.RequiredStatusChecks = &github.RequiredStatusChecks{
-		EnforcementLevel: github.String("everyone"),
-		Contexts:         &[]string{"continous-integration"},
+	branch.Protected = github.Bool(true)


Is this line necessary?

Removed the line branch.Protected is true.

dmitshur · 2017-01-10T18:24:26Z

tests/integration/repos_test.go

-		t.Fatalf("RequiredStatusChecks.Contexts should be: %v but is: %v", wantedContexts, *branch.Protection.RequiredStatusChecks.Contexts)
+	if !reflect.DeepEqual(*protection.RequiredStatusChecks.Contexts, wantedContexts) {
+		t.Fatalf("RequiredStatusChecks.Contexts should be: %v but is: %v", wantedContexts, *protection.RequiredStatusChecks.Contexts)
+	}


Given that you're checking reflect.DeepEqual(protection, want) below, this reflect.DeepEqual(*protection.RequiredStatusChecks.Contexts, wantedContexts) check is also redundant.

dmitshur · 2017-01-10T18:25:44Z

tests/integration/repos_test.go

-		t.Fatalf("RequiredStatusChecks should be enabled for everyone, set for: %v", *branch.Protection.RequiredStatusChecks.EnforcementLevel)
+	if !*protection.RequiredStatusChecks.IncludeAdmins {
+		t.Fatalf("Branch %v of repo %v does not enforce the required status checks on the repository admins", "master", *repo.Name)
 	}


Given that you're checking reflect.DeepEqual(protection, want) below, this protection.RequiredStatusChecks.IncludeAdmins check is redundant.

dmitshur · 2017-01-10T18:27:21Z

tests/integration/repos_test.go

+	}
+	if !reflect.DeepEqual(protection, want) {
+		t.Errorf("Repositories.GetBranchProtection returned %+v, want %+v", protection, want)
 	}


Is this actually passing? Just want to confirm.

@shurcooL : The API is failing,
I tried curl, but then realised the header needs to be set.

Is there a way to check messages within an err?

This is the message when I run the code also. Is there something I am missing.

repos_test.go:138: Repositories.UpdateBranchProtection() returned error: PUT https://api.github.com/repos/:userloginName/test-6382800227808658932/branches/master/protection: 422 Validation Failed [{Resource: Field: Code: Message:}]

The report_test.go file in GitHub folder passes.
Is there something i am missing in the call.

Can i track this as an issue and work on it?

According to https://developer.github.com/v3/#client-errors:

Sending invalid fields will result in a 422 Unprocessable Entity response.

Add some printf statements to see what the errors are in the response.

You might want to rebase your changes on top of the latest master to make sure you're not missing out on something that changed recently.

I added few printfs and found that the request used by repos_test.go in GitHub folder and one in integration have the same request details. So local calls went fine, but remote call failed.

This is the one request from repos_test inside GitHub folder
PUT /repos/o/r/branches/b/protection HTTP/1.1
Host: 127.0.0.1:54098
Accept: application/vnd.github.loki-preview+json
Content-Type: application/json
User-Agent: go-github/2

{"required_status_checks":{"include_admins":true,"strict":true,"contexts":["continuous-integration"]},"required_pull_request_reviews":{"include_admins":true},"restrictions":{"users":["u"],"teams":["t"]}}

This is the request from repos_test inside integration folder
PUT /repos/nrsingh/test-7504504064263669287/branches/branch1/protection HTTP/1.1
Host: api.github.com
Accept: application/vnd.github.loki-preview+json
Content-Type: application/json
User-Agent: go-github/2

{"required_status_checks":{"include_admins":true,"strict":true,"contexts":["continuous-integration"]},"required_pull_request_reviews":{"include_admins":true},"restrictions":{"users":["nrsingh"],"teams":["nrsinghtest"]}}

Ok I think I got it, this is the response I get
mode=block\r\n\r\n{"message":"Validation Failed","errors":["Only organization repositories can have users and team restrictions"]

Yes this is the issue, if the repository is chosen from the organisation, this works. I will make changes and submit.

dmitshur · 2017-01-12T19:49:04Z

tests/integration/repos_test.go

 	"reflect"
 	"testing"
-
-	"github.com/google/go-github/github"


No reason to change the above. The previous version was compatible with goimports, the new one isn't.

All imports have been ordered. The test cases have now RequiredPullRequestReviews also.

varadarajana · 2017-01-15T16:33:18Z

@shurcooL Are the changes fine?

dmitshur

I think it's just these 2 things (see inline comments) left.

dmitshur · 2017-01-16T05:38:48Z

tests/integration/repos_test.go

+
+	protection, _, err := client.Repositories.UpdateBranchProtection(*repo.Owner.Login, *repo.Name, "master", protectionRequest)
+
+	if err != nil {


Remove the blank line between the protection, _, err := ... line and if err != nil {. That way it's more visible that the error value is being checked. The blank line between those 2 lines is not helpful.

@shurcooL I have removed the line too.

dmitshur · 2017-01-16T05:41:20Z

tests/integration/repos_test.go

+		RequiredStatusChecks: &github.RequiredStatusChecks{
+			IncludeAdmins: github.Bool(true),
+			Strict:        github.Bool(true),
+			Contexts:      &[]string{"continuous-integration"},


@varadarajana, take a look at commit 5e7fada that was merged recently.

It changes these fields from pointers to values. So your code will need to be updated, otherwise this will fail to build if we merge it as is now.

I recommend rebasing this PR on top of latest master in order to be able to test this works correctly.

@shurcooL I have taken the latest master where the fields have changed. Incorporated these in the latest commit.

dmitshur

I've tested out the latest version.

It builds successfully, so that's good.

However, the test doesn't pass for me. I get:

=== RUN   TestRepositories_EditBranches
--- FAIL: TestRepositories_EditBranches (6.21s)
	repos_test.go:141: Repositories.UpdateBranchProtection() returned error: PUT https://api.github.com/repos/shurcooL-test/test-6129484611666145821/branches/master/protection: 422 Validation Failed [{Resource: Field: Code: Message:}]
FAIL
exit status 1
FAIL	github.com/google/go-github/tests/integration	6.220s

Edit: I dumped the response, and it looks like this:

{
	"message": "Validation Failed",
	"errors": [
		"Only organization repositories can have users and team restrictions"
	],
	"documentation_url": "https://developer.github.com/v3/repos/branches/#update-branch-protection"
}

Edit 2: I've sent GitHub support a question about this preview API to learn if there's a problem with the error response schema that they need to fix, because it doesn't seem to align with what https://developer.github.com/v3/#client-errors says, and github parses it poorly now (422 Validation Failed [{Resource: Field: Code: Message:}]).

dmitshur · 2017-01-17T17:32:17Z

tests/integration/repos_test.go

+		RequiredStatusChecks: &github.RequiredStatusChecks{
+			IncludeAdmins: true,
+			Strict:        true,
+			Contexts:      []string{"continuous-integration"},


~~There's a typo here. It was continous-integration previously, but you've changed it to continuous-integration (extra u). I don't think that's right.~~

Ah, I see this is actually a typo fix. Never mind, keep this as is. Sorry about my mistake.

dmitshur · 2017-01-17T17:52:18Z

See my review above.

Does anyone know if the integration tests have support for creating organization repositories?

As far as I know, that's not the case. I don't think we need to add support for that as part of this PR.

So in order to make the tests pass, let's get rid of the github.ProtectionRequest.Restrictions value and set it to nil:

@@ -130,10 +130,10 @@ func TestRepositories_EditBranches(t *testing.T) {
 		RequiredPullRequestReviews: &github.RequiredPullRequestReviews{
 			IncludeAdmins: true,
 		},
-		Restrictions: &github.BranchRestrictionsRequest{
-			Users: []string{*repo.Owner.Login},
-			Teams: []string{"team"},
-		},
+		// TODO: Only organization repositories can have users and team restrictions.
+		//       In order to be able to test these Restrictions, need to add support
+		//       for creating temporary organization repositories.
+		Restrictions: nil,
 	}
 
 	protection, _, err := client.Repositories.UpdateBranchProtection(*repo.Owner.Login, *repo.Name, "master", protectionRequest)
@@ -150,14 +150,7 @@ func TestRepositories_EditBranches(t *testing.T) {
 		RequiredPullRequestReviews: &github.RequiredPullRequestReviews{
 			IncludeAdmins: true,
 		},
-		Restrictions: &github.BranchRestrictions{
-			Users: []*github.User{
-				{Login: github.String(*repo.Owner.Login), ID: github.Int(1)},
-			},
-			Teams: []*github.Team{
-				{Slug: github.String("team"), ID: github.Int(2)},
-			},
-		},
+		Restrictions: nil,
 	}
 	if !reflect.DeepEqual(protection, want) {
 		t.Errorf("Repositories.UpdateBranchProtection() returned %+v, want %+v", protection, want)

After I've made that change, the test TestRepositories_EditBranches passed successfully for me.

How does that sound @varadarajana? If you're in agreement, please make that change, ~~fix the typo,~~ and this LGTM.

varadarajana · 2017-01-18T00:08:40Z

@shurcooL yes, i had to change the test data in order to make it pass earlier. The validation failure was referring to org level repositories as i had mentioned in the earlier. Now I have set the restrictions to nil and submitted.

dmitshur

LGTM. Thanks @varadarajana!

@gmlewis, do you wanna look at this before I merge it (absolutely squashing into one commit)? This PR fixes a build error (due to changed APIs) for integration tests.

dmitshur · 2017-01-18T03:07:33Z

@gmlewis, can you confirm that the CLA looks good for this PR and I can merge safely? @googlebot found some mismatch in the emails, and I'm not sure if that's been resolved since then.

gmlewis · 2017-01-18T03:12:12Z

On Android now... I will check later tonight.

gmlewis · 2017-01-18T05:00:10Z

Unfortunately, we are not able to accept this PR as-is.

According to: https://github.com/google/go-github/pull/509.patch
the first two commits are created by:
From: Varadarajan Aravamudhan <varadarajanaravamudhan@Varadarajans-MacBook-Pro.local>
which does not match the account used to sign the CLA.

If you, @varadarajana, perform a rebase on the master and squash all the 17 commits into a single commit and then force push your PR, it should use your newer email address and then this PR should verify the CLA check.

I apologize for the hassle, but that should fix it. Thank you, @varadarajana!

gmlewis

Requesting @varadarajana to rebase (if necessary) and squash all commits to resolve CLA issue.

varadarajana · 2017-01-18T16:53:23Z

@gmlewis I have now squashed all changes in one file and also managed few merge conflicts. Does this look good?

gmlewis · 2017-01-18T17:27:27Z

Unfortunately, that didn't work. There are still 3 commits and if you look at:
https://patch-diff.githubusercontent.com/raw/google/go-github/pull/509.patch
and search for From: you can see how each commit is attributed.

varadarajana · 2017-01-19T00:51:35Z

@shurcooL @gmlewis Can i close this PR, open a new PR with my changes from the master and submit again with changes for review. Now in my gitlog i do not see my commits at all

dmitshur · 2017-01-19T01:45:30Z

@gmlewis, since we're going to squash all the commits into one when merging anyway, can we do that and pick the commit author from one of the commits that has a CLA signed (i.e. Varadarajan Aravamudhan <varadaraajan@gmail.com>)? I think the stray commit would only be a problem if we merged them as is, without squashing. But this is just a theory, I'm not a lawyer.

gmlewis · 2017-01-19T18:02:36Z

I'll defer to @willnorris on how to proceed.

gmlewis · 2017-01-19T18:52:42Z

@willnorris has informed me that I can fully take care of this with the squashing myself, so you are correct, @shurcooL.
LGTM.
Will merge after my next meeting.

gmlewis · 2017-01-19T19:31:38Z

Wait a second... I'm now digging into this more deeply and it seems to me that recently-added documentation to github/repos.go is being removed here.
Is that what we really want, @shurcooL?

I think not. It appears that a rebase/merge went bad.

dmitshur · 2017-01-19T21:53:04Z

You're right, we don't want to include that change.

It wasn't there when I gave my LGTM, it came up after due to a bad rebase/merge.

I can clean it up though.

This change fixes the build errors due to changes in branch protection API, and allows TestRepositories_EditBranches to pass. Resolves google#507. Updates google#310.

googlebot · 2017-01-19T22:17:42Z

CLAs look good, thanks!

dmitshur · 2017-01-19T22:23:15Z

Fixed the problem, merging now (since this change had both our LGTMs at that time, and the only remaining issue with CLA has been resolved now).

dmitshur · 2017-01-19T23:10:43Z

For posterity, I'll mention that I ran the integration test and it succeeded, but sometimes I had to add a small time.Sleep(time.Second) delay after the repo creation, otherwise I'd get a failure unrelated to what the test tests (404 repo doesn't exist).

varadarajana · 2017-01-20T02:09:28Z

@shurcooL @gmlewis thank you.

@shurcooL. I use an account with Auth token and it works for me. I will check this and open up a new issue if needed.

dmitshur reviewed Jan 8, 2017

View reviewed changes

dmitshur requested changes Jan 8, 2017

View reviewed changes

dmitshur reviewed Jan 9, 2017

View reviewed changes

dmitshur requested changes Jan 9, 2017

View reviewed changes

dmitshur requested changes Jan 10, 2017

View reviewed changes

dmitshur mentioned this pull request Jan 11, 2017

Adds required pull request reviews #512

Closed

dmitshur reviewed Jan 12, 2017

View reviewed changes

dmitshur requested changes Jan 16, 2017

View reviewed changes

dmitshur reviewed Jan 17, 2017

View reviewed changes

dmitshur changed the title ~~Fix for #502 issue : sync up with new GitHub API~~ Update integration tests for new branch protection API. Jan 18, 2017

dmitshur approved these changes Jan 18, 2017

View reviewed changes

gmlewis approved these changes Jan 18, 2017

View reviewed changes

gmlewis requested changes Jan 18, 2017

View reviewed changes

varadarajana force-pushed the master branch from efea7d9 to 37c56b2 Compare January 18, 2017 16:10

gmlewis approved these changes Jan 19, 2017

View reviewed changes

gmlewis added lgtm and removed lgtm labels Jan 19, 2017

tests/integration: Update for new branch protection API.

f2ca5b4

This change fixes the build errors due to changes in branch protection API, and allows TestRepositories_EditBranches to pass. Resolves google#507. Updates google#310.

dmitshur force-pushed the master branch from ba4b90d to f2ca5b4 Compare January 19, 2017 22:17

dmitshur merged commit f2ca5b4 into google:master Jan 19, 2017


		protection, _, err := client.Repositories.UpdateBranchProtection(repo.Owner.Login, repo.Name, "master", protectionRequest)

		if err != nil {

Conversation

varadarajana commented Jan 8, 2017 • edited by dmitshur Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Jan 8, 2017

Uh oh!

varadarajana commented Jan 8, 2017

Uh oh!

googlebot commented Jan 8, 2017

Uh oh!

varadarajana commented Jan 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varadarajana commented Jan 9, 2017

Uh oh!

dmitshur commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varadarajana commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varadarajana commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varadarajana Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varadarajana commented Jan 8, 2017 •

edited by dmitshur

Loading

dmitshur Jan 9, 2017 •

edited

Loading

dmitshur commented Jan 9, 2017 •

edited

Loading

dmitshur Jan 9, 2017 •

edited

Loading

varadarajana Jan 11, 2017 •

edited

Loading

dmitshur Jan 16, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading

dmitshur Jan 17, 2017 •

edited

Loading