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.

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

Add integration test for repository migration #1983

Merged
merged 1 commit into from
Jun 20, 2017
Merged

Add integration test for repository migration #1983

merged 1 commit into from
Jun 20, 2017

Conversation

typeless
Copy link
Contributor

@typeless typeless commented Jun 16, 2017 •
edited
Loading

A build flag 'extra' for guarding the test since it would take long time and consume too much bandwidth. It can be used as a building block for other tests.

P.S. It's changed per the suggestions by the reviewers.

@lunny lunny added this to the 1.2.0 milestone Jun 16, 2017
sapk
sapk reviewed Jun 16, 2017
View reviewed changes
integrations/repo_migrate_test.go Outdated
func TestRepoMigrate(t *testing.B) {
prepareTestEnv(t)
session := loginUser(t, "user2", "password")
testRepoMigrate(t, session, "git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git")
Copy link
Member

Choose a reason for hiding this comment

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

We could maybe test with a smaller repo (like gitea ^^) and benchmark with biggest one ?

Copy link
Member

Choose a reason for hiding this comment

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

For benchmark you could use https://github.com/go-gitea/git/blob/master/tree_entry_test.go as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sapk I was trying to follow the example, but I can't figure out how to make the integration framework fit into a benchmark function. The APIs of integration_test requires a testing.T but a benchmark function provides only test.B. :trollface:

@typeless typeless force-pushed the add-integration-test-for-migration branch from f4129a2 to ed0c698 Compare June 16, 2017 14:27
@ethantkoenig
Copy link
Member

As a heads up, if #1979 gets merged before this, be sure to rebase.

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

lafriks commented Jun 17, 2017

#1979 is merged so this needs to be updated with changed methods (login/csrf/request)

@typeless typeless force-pushed the add-integration-test-for-migration branch from ed0c698 to 1d10f0a Compare June 17, 2017 07:12
@typeless
Copy link
Contributor Author

@lafriks Done. The benchmark is not implemented for now as I have not idea how to apply the integration APIs in a benchmark function.

lafriks
lafriks requested changes Jun 17, 2017
View reviewed changes
integrations/repo_migrate_test.go Outdated
repoName := repoDirname[0 : len(repoDirname)-len(path.Ext(repoDirname))]

req = NewRequestWithValues(t, "POST", link, map[string]string{
"_csrf": htmlDoc.GetInputValueByName("_csrf"),
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be changed to htmlDoc.GetCsrf()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@typeless typeless force-pushed the add-integration-test-for-migration branch from 1d10f0a to 936d1fc Compare June 17, 2017 07:21
@sapk
Copy link
Member

sapk commented Jun 17, 2017

@typeless we should convert to the common interface https://golang.org/pkg/testing/#TB for allowing benchmark. I will try to setup something.

@sapk sapk mentioned this pull request Jun 17, 2017
Merged
@typeless typeless force-pushed the add-integration-test-for-migration branch from 936d1fc to 0c512df Compare June 17, 2017 15:12
@typeless
Copy link
Contributor Author

Benchmarks have been added thanks to #1993 .

sapk
sapk reviewed Jun 17, 2017
View reviewed changes
integrations/repo_migrate_test.go Outdated
session := loginUser(b, "user2")

for _, s := range samples {
b.StopTimer()
Copy link
Member

Choose a reason for hiding this comment

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

This should be the first line or at least before prepareTestEnv(b)

Copy link
Member

Choose a reason for hiding this comment

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

For details : b.StopTimer() permit to ingore setup time. So in this case the only setup part is L58-59. Even b.StartTimer() don't need to be in the loop for this case since we only setup once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@sapk
Copy link
Member

sapk commented Jun 17, 2017 •
edited
Loading

A little change to make. The rest LGTM.

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 17, 2017
@typeless typeless force-pushed the add-integration-test-for-migration branch from 0c512df to ef35974 Compare June 17, 2017 16:40
@lafriks
Copy link
Member

lafriks commented Jun 17, 2017

Need to rebase as some functions were renamed

@typeless typeless force-pushed the add-integration-test-for-migration branch from ef35974 to b0f2270 Compare June 17, 2017 17:01
@typeless
Copy link
Contributor Author

@lafriks Done.

@lafriks
Copy link
Member

lafriks commented Jun 19, 2017 •
edited
Loading

Can you add task for running benchmark to Makefile?

@typeless typeless force-pushed the add-integration-test-for-migration branch from b0f2270 to 83cceaa Compare June 20, 2017 02:08
@typeless
Copy link
Contributor Author

@lafriks Done.

@lafriks
Copy link
Member

lafriks commented Jun 20, 2017

Have not tested but LGTM

@tboerger tboerger removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jun 20, 2017
@tboerger tboerger added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Jun 20, 2017
@lunny lunny merged commit 754482b into go-gitea:master Jun 20, 2017
@typeless typeless deleted the add-integration-test-for-migration 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.
Reviewers

@sapk sapk sapk left review comments

@lafriks lafriks lafriks approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/testing
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
6 participants
@typeless @ethantkoenig @lafriks @sapk @lunny @tboerger