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

Add integration test for file editing #1907

Merged
merged 1 commit into from
Jun 8, 2017

Conversation

typeless
Copy link
Contributor

@typeless typeless commented Jun 8, 2017

No description provided.

@typeless typeless force-pushed the add-integration-test-for-edit-file branch 2 times, most recently from 3cca1c4 to a436302 Compare June 8, 2017 03:15
@appleboy
Copy link
Member

appleboy commented Jun 8, 2017

LGTM

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 8, 2017
Copy link
Member

@bkcsoft bkcsoft left a 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

@@ -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) {
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

func TestEditFile(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user2", "password")
testEditFile(t, session, "/user2/repo1/master/README.md")
Copy link
Member

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

🙂

Copy link
Contributor Author

@typeless typeless Jun 8, 2017

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.

Copy link
Member

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 :)

@typeless typeless force-pushed the add-integration-test-for-edit-file branch from a436302 to fb68890 Compare June 8, 2017 05:23
@lunny lunny added this to the 1.2.0 milestone Jun 8, 2017
@lafriks
Copy link
Member

lafriks commented Jun 8, 2017

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 8, 2017
@lunny lunny merged commit 662b8bc into go-gitea:master Jun 8, 2017
@typeless typeless deleted the add-integration-test-for-edit-file branch April 3, 2019 05:00
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants