Add basic integration test infrastructure (and new endpoint `/api/v1/version` for testing it) #741

typeless · 2017-01-24T08:47:52Z

The primary effort of the PR is to implement a prototype of (poor man's) automated functional tests for Gitea. It currently tests /install and /version only.

For #63

typeless · 2017-01-24T14:36:54Z

@lunny This is basically functioning and ready for review now.

lunny · 2017-01-25T15:46:47Z

Makefile

@@ -67,7 +67,8 @@ lint:
 	for PKG in $(PACKAGES); do golint -set_exit_status $$PKG || exit 1; done;

 .PHONY: test
-test:
+test: TAGS=bindata sqlite


@tboerger maybe you can review this file changed.

For the record, I used to add a new target like xtest: specifically for the new testing facility but go test would still look into the directory and keep doing its thing. Another approach to prevent it is to deliberately exclude the /tests/ directory from the PACKAGE variable for the xtest target.

Any ideas?

Edit: @lunny @tboerger I've changed it per the aforementioned way.

ethantkoenig · 2017-01-29T04:18:20Z

tests/internal/utils/utils.go

+}
+
+// RunTest Helper function for setting up a running Gitea server for functional testing and then gracefully terminating it.
+func (c *Config) RunTest(tests ...func(*Config) error) (err error) {


Would it make sense to have the functions in tests also take a *testing.T argument? That way, the tests in tests can call assert.Equal(..) and the like.

Agreed. I'll submit a change later for evaluation. And if it works well, I'll then adopt the assert.Equal way.

Edit: I have submitted the proposed change.

ethantkoenig · 2017-01-29T04:21:32Z

tests/version_test.go

+
+	log.Printf("Actual: \"%s\" Expected: \"%s\"\n", string(actual), string(expected))
+	if actual != expected {
+		return fmt.Errorf("do not match")


This error message isn't very specific; if I see the error message "do not match", I don't know which test failed, why it failed (other than that something didn't match something else).

Like I said above, if we could find a way to pass a *testing.T into the tests, that would be really nice.

ethantkoenig · 2017-01-31T20:04:50Z

routers/api/v1/misc/version.go

@@ -0,0 +1,11 @@
+package misc


Need to include copyright info

Done. 069155d249b94bb89a84a27002db177c072672c1

ethantkoenig · 2017-01-31T20:05:59Z

tests/internal/utils/utils.go

+	LogFile *os.File
+}
+
+func redirect(cmd *exec.Cmd, f *os.File) error {


The f argument is not used.

Done. a68c7ae

ethantkoenig · 2017-01-31T20:08:49Z

tests/install_test.go

+
+		// Give the server some amount of time to warm up.
+		time.Sleep(100 * time.Millisecond)
+		fmt.Fprintf(os.Stderr, ".")


Can just use fmt.Fprint(os.Stderr, ".")

Without the sleeps, the retry count would be around ~250 on my laptop. And I think the . with a fixed amount of timeout can give a rough metric on how the server startup time changes a long the way. If scalability really become an issue, we can change the timeout then.

No, I meant keep the sleep, but replace the call to fmt.Fprintf(..) (line 50) with fmt.Fprint(..). No reason to use fmt.Fprintf(..) if you're not using any formatting. Sorry for any confusion.

ethantkoenig · 2017-01-31T20:10:14Z

tests/install_test.go

+	}
+	defer resp.Body.Close()
+
+	_ = resp


Is this line necessary?

Ah, it was for making debugging easier. OK to remove it now.

ethantkoenig · 2017-01-31T20:14:27Z

tests/version_test.go

+
+	actual := string(bytes.TrimSpace(buf))
+
+	log.Printf("Actual: \"%s\" Expected: \"%s\"\n", string(actual), string(expected))


actual and expected are already strings; they don't need to be casted.

ethantkoenig · 2017-01-31T20:19:57Z

tests/internal/utils/utils.go

+
+	workdir := t.Config.WorkDir
+	if workdir == "" {
+		workdir, err = filepath.Abs(fmt.Sprintf("%s-%10d", filepath.Base(t.Config.Program), time.Now().UnixNano()))


I'd prefer something like ioutil.TempDir(os.TempDir(), "gitea_tests"). Since we're going to discard the contents of workdir anyways (line 78), we might as well use a temporary directory.

ethantkoenig · 2017-01-31T20:21:44Z

tests/version_test.go

+		//LogFile: os.Stderr,
+	}
+
+	if err := utils.New(t, &conf).RunTest(install, version); err != nil {


It would be nice if we could find a way to not have to run install before every test. Once we start adding more end-to-end tests, this won't scale very well.

I am thinking about adding a method func (t *T) Cleanup() to do the housekeeping. The tests we currently have would then look like utils.New(t, conf).RunTest(install, version).Cleanup().

But when used as follows to 'share' a testing instance across many testing functions:

var globalT *utils.T func init() { globalT = utils.New(t.conf).RunTest(install) } func TestA(t *testing.T) { globalT.RunTest(version) } func TestB(t *testing.T) { globalT.RunTest(signup) }

I have no idea how we can call the cleanup method before quitting.

bkcsoft · 2017-02-06T21:01:20Z

I don't see any unit-tests for /version ;P

Also please don't call it 'xtest', go for 'integration-test' or 'e2e-test' :)

typeless · 2017-02-08T05:49:12Z

@bkcsoft What do you mean by the 'unit-tests'? There isn't any under routers that I can copy either so I don't understand how it would be done.

lunny · 2017-02-08T06:29:40Z

build failed.

typeless · 2017-02-08T07:08:15Z

@lunny It's strange. My local copy doesn't have the problem and I can't see how this change could break it. I just renamed a Makefile target irrelevant to the unit tests.

lunny · 2017-02-08T07:30:25Z

Please rebase.

bkcsoft

I didn't go through the tests, but they seem to pass 😉

bkcsoft · 2017-02-12T15:06:29Z

routers/api/v1/misc/version.go

+
+// ServerVersion shows the version of the Gitea server
+func ServerVersion(ctx *context.APIContext) {
+	ctx.Write([]byte(setting.AppVer))


As the entire API is JSON, maybe you should do this instead

type serverVersion struct { Version string } func ServerVersion(ctx *context.APIContext) { ctx.JSON(200, &serverVersion{Version: setting.AppVer}) }

Preferably the struct should go in code.gitea.io/sdk 🙂 (If you do that just call it api.Version)

There exists an API named Version already (https://github.com/go-gitea/gitea/blob/master/vendor/code.gitea.io/sdk/gitea/gitea.go#L17) for library's version.

I am afraid that it would lead to confusion if I have the both called 'Version'.
I did change the function name of the server-side handler to Version. PTAL.

lunny · 2017-02-22T12:26:00Z

@typeless is this ready to review now?

typeless · 2017-02-22T13:47:23Z

@lunny Yes.

appleboy · 2017-02-22T14:31:11Z

tests/version_test.go

+
+	"github.com/stretchr/testify/assert"
+
+	"code.gitea.io/gitea/tests/internal/utils"


https://github.com/go-gitea/gitea/blob/master/CONTRIBUTING.md#styleguide

import ( // stdlib "encoding/json" "fmt" // local packages "code.gitea.io/gitea/models" "code.gitea.io/sdk/gitea" // external packages "github.com/foo/bar" "gopkg.io/baz.v1" )

ethantkoenig · 2017-02-22T22:07:52Z

tests/version_test.go

+
+	fields := strings.Fields(string(out))
+	if len(fields) != 3 {
+		return fmt.Errorf("unexpected version string")


Minor, but could we include the unexpected version string in the error message; it would make debugging easier in the case where there is unexpected output.

typeless · 2017-02-23T01:46:41Z

For the record I just found the tests can not pass if using the Go tip, but works fine with Go 1.8. 😮

EDIT: Git bisect shows that it's golang/go@3792db5

strk

Could integration tests be put under a specific directory rather than just the generic /test/ one ?

strk · 2017-02-23T06:56:18Z

tests/install_test.go

+
+func makeSimpleSettings(user, workdir, port string) map[string][]string {
+	return map[string][]string{
+		"db_type":        {"SQLite3"},


Doesn't this need a build tag to work ?
Any chance to test other databases too ?

Doesn't this need a build tag to work ?

Yes. Currently we need to have an existing Gitea binary built with SQLite3 to work. I have added a Makefile target named 'e2e-test' to simplify the steps.

Any chance to test other databases too ?

I have not used other databases for Gitea so far and they are much harder to setup. Therefore I would delegate the task to other developers with the skills & experiences.

Also change back the Makefile to use `make integration-test`.

Slightly flattened directory hierarchy is better.

lunny · 2017-03-04T15:39:40Z

integrations/install_test.go

+	return map[string][]string{
+		"db_type":        {"SQLite3"},
+		"db_host":        {"localhost"},
+		"db_path":        {workdir + "data/gitea.db"},


But the test db data will stay with production data?

The 'workdir' is created by ioutil.TempDir if not specified (that is, an empty string).

My comment regarding the field WorkDir is outdated and does not match with the reality. I'll fix it. Thanks.

strk · 2017-03-04T17:30:26Z

On Sat, Mar 04, 2017 at 06:14:01AM -0800, Mura Li wrote: Vendoring configuration has been updated.

Was the PR merged already ? Can you drop a link to the sdk PR here ?

typeless · 2017-03-05T01:51:49Z

Was the PR merged already ? Can you drop a link to the sdk PR here ?

go-gitea/go-sdk#43

andreynering · 2017-03-05T20:42:50Z

LGTM

Just missing @bkcsoft approval.

typeless · 2017-03-06T02:27:24Z

@bkcsoft Please take a look at the PR.

lunny · 2017-03-07T02:10:20Z

This will produce integrationsdata, integrationslog and integrationsrepositories, that should be a bug. @typeless

typeless changed the title ~~Add endpoint for getting the server version~~ [WIP] Add endpoint for getting the server version Jan 24, 2017

lunny added this to the 1.1.0 milestone Jan 24, 2017

lunny added pr/wip This PR is not ready for review modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Jan 24, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 24, 2017

typeless changed the title ~~[WIP] Add endpoint for getting the server version~~ Add endpoint for getting the server version Jan 24, 2017

lunny removed the pr/wip This PR is not ready for review label Jan 24, 2017

lunny reviewed Jan 25, 2017

View reviewed changes

typeless mentioned this pull request Jan 26, 2017

Improve the performance of pull-request merging #641

Closed

ethantkoenig suggested changes Jan 29, 2017

View reviewed changes

ethantkoenig suggested changes Jan 31, 2017

View reviewed changes

typeless force-pushed the add-endpoint-for-version branch from ccbf863 to c6fcd32 Compare February 8, 2017 07:45

bkcsoft suggested changes Feb 12, 2017

View reviewed changes

typeless changed the title ~~Add endpoint for getting the server version~~ Add basic integration test (and new endpoint /api/v1/version for testing it) Feb 22, 2017

typeless changed the title ~~Add basic integration test (and new endpoint /api/v1/version for testing it)~~ Add basic integration test infrastructure (and new endpoint /api/v1/version for testing it) Feb 22, 2017

appleboy reviewed Feb 22, 2017

View reviewed changes

ethantkoenig suggested changes Feb 22, 2017

View reviewed changes

lunny added the type/testing label Feb 23, 2017

typeless force-pushed the add-endpoint-for-version branch from 0188e04 to f320057 Compare February 23, 2017 02:20

strk suggested changes Feb 23, 2017

View reviewed changes

typeless added 8 commits March 4, 2017 22:16

Rename 'integration-test' to 'e2e-test' for saving keystrokes

a81ef4c

Add comment to avoid possible confusion

6a60048

Rename tests -> integration-tests

175267d

Also change back the Makefile to use `make integration-test`.

Use tests/integration for now

2532c60

tests/integration -> integrations

3430d3a

Slightly flattened directory hierarchy is better.

Update Makefile accordingly

6dedc14

Fix a missing change in Makefile

e4ad394

govendor update code.gitea.io/sdk/gitea

b022db5

typeless force-pushed the add-endpoint-for-version branch from 04131a2 to b022db5 Compare March 4, 2017 14:18

lunny requested changes Mar 4, 2017

View reviewed changes

typeless added 6 commits March 5, 2017 10:10

Fix comment of struct fields

7fe9959

Fix conditional nonsense

a7547bf

Fix missing updates regarding version string changes

01a2a73

Make variable naming more consistent

55e54c3

Check http status code

22a7ad4

Rectify error messages

3223af5

lunny approved these changes Mar 5, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 5, 2017

bkcsoft approved these changes Mar 6, 2017

View reviewed changes

lunny merged commit 8482936 into go-gitea:master Mar 6, 2017

typeless mentioned this pull request Mar 7, 2017

Fix stray directories generated by integration tests #1134

Merged

lunny mentioned this pull request Mar 17, 2017

Integration test framework #1290

Merged

typeless deleted the add-endpoint-for-version branch April 3, 2019 04:59

go-gitea locked and limited conversation to collaborators Nov 24, 2020


		actual := string(bytes.TrimSpace(buf))

		log.Printf("Actual: \"%s\" Expected: \"%s\"\n", string(actual), string(expected))


		"github.com/stretchr/testify/assert"

		"code.gitea.io/gitea/tests/internal/utils"

Add basic integration test infrastructure (and new endpoint /api/v1/version for testing it) #741

Add basic integration test infrastructure (and new endpoint /api/v1/version for testing it) #741

Conversation

typeless commented Jan 24, 2017 • edited

typeless commented Jan 24, 2017

Choose a reason for hiding this comment

typeless Jan 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeless Jan 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeless Feb 1, 2017 • edited

Choose a reason for hiding this comment

bkcsoft commented Feb 6, 2017

typeless commented Feb 8, 2017

lunny commented Feb 8, 2017

typeless commented Feb 8, 2017

lunny commented Feb 8, 2017

bkcsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeless Feb 21, 2017 • edited

Choose a reason for hiding this comment

lunny commented Feb 22, 2017

typeless commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeless commented Feb 23, 2017 • edited

strk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeless Feb 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strk commented Mar 4, 2017 via email

typeless commented Mar 5, 2017 • edited

andreynering commented Mar 5, 2017 • edited

typeless commented Mar 6, 2017

lunny commented Mar 7, 2017

Add basic integration test infrastructure (and new endpoint `/api/v1/version` for testing it) #741

Add basic integration test infrastructure (and new endpoint `/api/v1/version` for testing it) #741

typeless commented Jan 24, 2017 •

edited

typeless Jan 26, 2017 •

edited

typeless Jan 29, 2017 •

edited

typeless Feb 1, 2017 •

edited

typeless Feb 21, 2017 •

edited

typeless commented Feb 23, 2017 •

edited

typeless Feb 23, 2017 •

edited

typeless commented Mar 5, 2017 •

edited

andreynering commented Mar 5, 2017 •

edited