Skip to content

Update integration tests for new branch protection API. #509

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.

Sign up for GitHub

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

Merged
dmitshur merged 1 commit into from
Jan 19, 2017
Merged

Update integration tests for new branch protection API. #509

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 29 additions & 16 deletions tests/integration/repos_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,30 +114,43 @@ func TestRepositories_EditBranches(t *testing.T) {
t.Fatalf("Repositories.GetBranch() returned error: %v", err)
}

if *branch.Protection.Enabled {
if *branch.Protected {
t.Fatalf("Branch %v of repo %v is already protected", "master", *repo.Name)
}

branch.Protection.Enabled = github.Bool(true)
branch.Protection.RequiredStatusChecks = &github.RequiredStatusChecks{
EnforcementLevel: github.String("everyone"),
Contexts: &[]string{"continous-integration"},
protectionRequest := &github.ProtectionRequest{
RequiredStatusChecks: &github.RequiredStatusChecks{
IncludeAdmins: true,
Strict: true,
Contexts: []string{"continuous-integration"},
Copy link
Copy Markdown
Member

@dmitshur dmitshur Jan 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@dmitshur dmitshur Jan 17, 2017

Choose a reason for hiding this comment

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

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

},
RequiredPullRequestReviews: &github.RequiredPullRequestReviews{
IncludeAdmins: true,
},
// 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,
}
branch, _, err = client.Repositories.EditBranch(*repo.Owner.Login, *repo.Name, "master", branch)

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

@dmitshur dmitshur Jan 16, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@varadarajana varadarajana Jan 16, 2017

Choose a reason for hiding this comment

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

@shurcooL I have removed the line too.

t.Fatalf("Repositories.EditBranch() returned error: %v", err)
t.Fatalf("Repositories.UpdateBranchProtection() returned error: %v", err)
}

if !*branch.Protection.Enabled {
t.Fatalf("Branch %v of repo %v should be protected, but is not!", "master", *repo.Name)
}
if *branch.Protection.RequiredStatusChecks.EnforcementLevel != "everyone" {
t.Fatalf("RequiredStatusChecks should be enabled for everyone, set for: %v", *branch.Protection.RequiredStatusChecks.EnforcementLevel)
want := &github.Protection{
RequiredStatusChecks: &github.RequiredStatusChecks{
IncludeAdmins: true,
Strict: true,
Contexts: []string{"continuous-integration"},
},
RequiredPullRequestReviews: &github.RequiredPullRequestReviews{
IncludeAdmins: true,
},
Restrictions: nil,
}

wantedContexts := []string{"continous-integration"}
if !reflect.DeepEqual(*branch.Protection.RequiredStatusChecks.Contexts, wantedContexts) {
t.Fatalf("RequiredStatusChecks.Contexts should be: %v but is: %v", wantedContexts, *branch.Protection.RequiredStatusChecks.Contexts)
if !reflect.DeepEqual(protection, want) {
t.Errorf("Repositories.UpdateBranchProtection() returned %+v, want %+v", protection, want)
}
Copy link
Copy Markdown
Member

@dmitshur dmitshur Jan 10, 2017

Choose a reason for hiding this comment

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

Is this actually passing? Just want to confirm.

Copy link
Copy Markdown
Contributor Author

@varadarajana varadarajana Jan 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Contributor Author

@varadarajana varadarajana Jan 11, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@dmitshur dmitshur Jan 12, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@varadarajana varadarajana Jan 14, 2017

Choose a reason for hiding this comment

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

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"]}}

Copy link
Copy Markdown
Contributor Author

@varadarajana varadarajana Jan 14, 2017

Choose a reason for hiding this comment

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

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"]

Copy link
Copy Markdown
Contributor Author

@varadarajana varadarajana Jan 14, 2017

Choose a reason for hiding this comment

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

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


_, err = client.Repositories.Delete(*repo.Owner.Login, *repo.Name)
Expand Down