-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Add integration test for file editing #1907
Add integration test for file editing #1907
Conversation
3cca1c4
to
a436302
Compare
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few suggestions for improvement
integrations/editor_test.go
Outdated
@@ -104,3 +106,55 @@ func TestCreateFileOnProtectedBranch(t *testing.T) { | |||
// Check body for error message | |||
assert.Contains(t, string(resp.Body), "Can not commit to protected branch 'master'.") | |||
} | |||
|
|||
func testEditFile(t *testing.T, session *TestSession, urlstr string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather change this to owner, repo, branch, filePath, content string
TBH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of that at first but another side of me told that function parameters should no more than 4 if possible otherwise it would look awkward. I will reconsider your advice or if anybody else there is willing to chime in? I don't strongly insist on either way but I'd like to have more confirmations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it can simplify the code handling the arguments, I am now inclined to agree with you.
integrations/editor_test.go
Outdated
func TestEditFile(t *testing.T) { | ||
prepareTestEnv(t) | ||
session := loginUser(t, "user2", "password") | ||
testEditFile(t, session, "/user2/repo1/master/README.md") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also check if the user:
- can edit a collaborating repo. (user does not own, but can push)
- can not edit a repo that they don't have push-right to.
- can not edit a protected branch
🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My plan was to have this as a stepstone for the pull-merge testing (#641). I'd like to leave those further works to other PRs if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is integration test for editing file in protected branch. I added it when fixing bug that did allow edit files on protected branch :)
a436302
to
fb68890
Compare
LGTM |
No description provided.