Skip to content

Added test case for commit ref with percent encoding #1118

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

Jump to bottom
Merged
merged 3 commits into from
Feb 24, 2019
Merged

Added test case for commit ref with percent encoding #1118

merged 3 commits into from
Feb 24, 2019

Conversation

vaibhavsingh97
Copy link
Contributor

@vaibhavsingh97 vaibhavsingh97 commented Feb 11, 2019 •
edited
Loading

Fixes #1101

Added test case for Repositories.GetCommitSHA1, when it was called with the branch name which includes the non-alphabet character.

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Feb 11, 2019
@gmlewis
Copy link
Contributor

gmlewis commented Feb 11, 2019

Thanks, @vaibhavsingh97. First, please don't use force-push for commits on PRs for this repo, as we "Squash and merge" them anyway.

Second, you may be aware of this already, but the Travis CI build is currently failing with the most recent commit, with this error:

--- FAIL: TestRepositoriesService_NonAlphabetCharacter_GetCommitSHA1 (0.00s)
    repos_commits_test.go:240: Repositories.GetCommitSHA1 returned error: GET http://127.0.0.1:46687/api-v3/repos/o/r/commits/master%2520hash: 404  []
    repos_commits_test.go:245: Repositories.GetCommitSHA1 = , want 01234abcde

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Googlers can find more info about SignCLA and this PR by following this link.

@googlebot googlebot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Feb 20, 2019
@vaibhavsingh97
Copy link
Contributor Author

vaibhavsingh97 commented Feb 20, 2019 •
edited
Loading

@gmlewis Sorry for late update. I had updated my PR, please review

Thanks, @vaibhavsingh97. First, please don't use force-push for commits on PRs for this repo, as we "Squash and merge" them anyway.

Thanks, I will keep in mind in near future

@gmlewis
Copy link
Contributor

gmlewis commented Feb 20, 2019

Hmmm... I'm not sure why GitHub is showing us such a mess in the diffs.
Why we are seeing v23 or v24 at all is a mystery to me. Normally, we see diffs against the master branch which already has v24 in it, so these diffs make no sense to me.
I'll have to study this again later tonight and figure out what is going on.

@vaibhavsingh97
Copy link
Contributor Author

@gmlewis I had fixed this but it's needed a force push, please let me know if I needed to do the same

@gmlewis
Copy link
Contributor

gmlewis commented Feb 20, 2019

OK that's odd. If you can fix this with force pushes, then please go ahead despite what I said earlier. 😄

@googlebot
Copy link

CLAs look good, thanks!

Googlers can find more info about SignCLA and this PR by following this link.

@googlebot googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Feb 20, 2019
@vaibhavsingh97
Copy link
Contributor Author

@gmlewis Please review 😄

gmlewis
gmlewis approved these changes Feb 22, 2019
View reviewed changes
Copy link
Contributor

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @vaibhavsingh97 and sorry for the delay.
LGTM apart from a tiny nit, if you don't mind addressing.

Awaiting second LGTM before merging.

github/repos_commits_test.go Outdated
}

want := sha1
if got != want {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Go style is typically:

if want := sha1; got != want {

although I know we don't always follow that style in this repo. I'd prefer it, though, if you don't mind and there aren't any objections.

github/repos_commits_test.go Outdated
}

want = ""
if got != want {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: if want := ""; got != want {

Copy link
Contributor Author

@vaibhavsingh97 vaibhavsingh97 Feb 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@gmlewis I added suggested changes, but it gave me error

github/repos_commits_test.go:260:5: undefined: want
github/repos_commits_test.go:261:61: undefined: want

should I revert it?

@gmlewis gmlewis requested a review from gauntface February 22, 2019 02:35
gmlewis
gmlewis reviewed Feb 23, 2019
View reviewed changes
github/repos_commits_test.go Outdated
t.Errorf("Expected HTTP 304 response")
}

if want = ""; got != want {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're just missing a : here:

if want := ""; got != want {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, missed it. Thanks, I had updated my PR

kshitij10496
kshitij10496 approved these changes Feb 24, 2019
View reviewed changes
Copy link
Contributor

@kshitij10496 kshitij10496 left a comment

Choose a reason for hiding this comment

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

LGTM, +1.
Good work @vaibhavsingh97! 🎉

gmlewis
gmlewis approved these changes Feb 24, 2019
View reviewed changes
Copy link
Contributor

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @vaibhavsingh97 and @kshitij10496 for the review.
Merging.

@gmlewis gmlewis merged commit e810655 into google:master Feb 24, 2019
@vaibhavsingh97 vaibhavsingh97 deleted the patch/url-escape-test branch February 25, 2019 05:39
@vetinari vetinari mentioned this pull request Feb 18, 2020
Merged
n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
@vaibhavsingh97 @gmlewis
Added test case for commit ref with percent encoding (google#1118)
3c8b1de 
Fixes google#1101.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gauntface gauntface Awaiting requested review from gauntface

2 more reviewers

@gmlewis gmlewis gmlewis approved these changes

@kshitij10496 kshitij10496 kshitij10496 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vaibhavsingh97 @gmlewis @googlebot @kshitij10496