Gitbase multi version testing #363

se7entyse7en · 2019-03-22T09:32:41Z

Closes #336.

This PR adds integration testing that tests the compatibility of gitbase across different version of engine:

make test-integration

Two versions are considered that are named PREV and NEXT. By default PREV is the latest non rc version and NEXT is HEAD. For testing it is also possible to pass this as env vars in order to test two arbitrary version of engine:

PREV_VERSION=v0.10.0 NEXT_VERSION=v0.11.0 make test-integration

se7entyse7en · 2019-03-22T09:41:56Z

I was thinking about a couple of things. Given that it is possible to test two given version of engine for retro-compatibility, and that this makes sense only for this kind of tests, maybe we could add another build flag such as integration-retro-comp and run them separately from the "simple" integration tests. So that we have:

make test-integration

and

make test-integration-retro-comp

WDYT?

se7entyse7en · 2019-03-22T09:43:25Z

BTW by running this:

PREV_VERSION=v0.10.0 NEXT_VERSION=v0.11.0 make test-integration

I was able to detect the backward incompatibility. 🎉
As other examples, both this:

PREV_VERSION=v0.9.0 NEXT_VERSION=v0.10.0 make test-integration

and this:

PREV_VERSION=v0.11.0 make test-integration

succeeded.

se7entyse7en · 2019-03-22T09:43:59Z

@smacker I didn't add the EXPLAIN TREE thing as I think that the current test is enough, WDYT?

se7entyse7en · 2019-03-22T10:01:38Z

mhmmm for some reason it didn't execute the integration tests on travs... I'll investigate.

smacker · 2019-03-22T10:36:27Z

I didn't add the EXPLAIN TREE thing as I think that the current test is enough, WDYT?

I would better add it to make sure that the index is actually used by gitbase. Otherwise, they may show index in "SHOW INDEXES" but because it's outdated/calculated for a different directory they skip it when executing the query. But it's not like VERY important.

Makefile

cmd/test-utils/common.go

cmd/srcd/cmd/gitbase_back_comp_test.go

carlosms · 2019-03-25T17:03:38Z

I have a similar comment to @smacker. I didn't test the current approach extensively, but my first impression is that it limits the usefulness of the integration tests in the local dev machine.
Using the current git to checkout older versions has these problems:

It does not return to the branch I was working on. Instead, it checks out the last commit.
If I have any local changes I cannot run the integration tests.

Did you check if we can use https://github.com/src-d/regression-core to download or build the binaries?

se7entyse7en · 2019-03-25T17:13:29Z

It does not return to the branch I was working on. Instead, it checks out the last commit.

true, this could be fixed easily I think.

If I have any local changes I cannot run the integration tests.

true, I think that this could be solved by stashing/unstashing.

But both things are actually not needed if we decide to test released versions only (which makes a lot of sense actually).

Did you check if we can use https://github.com/src-d/regression-core to download or build the binaries?

I forgot about it 😕. I'm gonna take a look into it 👍.

se7entyse7en · 2019-03-26T10:09:37Z

This is currently on hold for this.

smacker · 2019-03-26T11:49:05Z

Given that this PR is already big, do you mind approaching this separately?

if we go with src-d/regression-core approach it will make sense to follow the same workflow as other regression tests (a separate binary that runs on jenkins).

Another thing if we embed struct correctly PR won't be that big & the code will be very localized without affecting anything else. So I don't think there should be a rush with merging.

se7entyse7en · 2019-03-26T11:51:03Z

So I don't think there should be a rush with merging.

👍

if we go with src-d/regression-core approach it will make sense to follow the same workflow as other regression tests (a separate binary that runs on jenkins).

Yup, I was thinking the same, we should discuss whether it makes more sense to do as you said.

.travis.yml

dpordomingo

some questions.
also after the long discussion, and the rebase push force I'm not sure about the state of this PR; if it already addressed al suggestions (on which I agree), or if there are still missing things.

cmd/srcd/cmd/gitbase_back_comp_test.go

se7entyse7en · 2019-03-27T16:02:39Z

also after the long discussion, and the rebase push force I'm not sure about the state of this PR

Yup sorry for the force push, but there were so many changes that it would make the following rebase much more difficult.

smacker

setupWd is a dirty hack but I think it's okay to keep it for now

se7entyse7en · 2019-03-29T09:37:07Z

setupWd is a dirty hack but I think it's okay to keep it for now

Yeah, I know 😞. I've realized later that go test sets the working dir to the package being tested, because on the other hand dlv test doesn't. We could eventually open an issue on regression-core to avoid assuming that it runs from the root of the project.

carlosms · 2019-04-02T12:29:01Z

What is the status of this PR? vendor is not updated. Is this because you are waiting on src-d/regression-core#8?

se7entyse7en · 2019-04-02T12:32:05Z

What is the status of this PR? vendor is not updated. Is this because you are waiting on src-d/regression-core#8?

Yup, I was able to run it locally by patching it.

smacker · 2019-04-02T13:19:55Z

pinged guys in #dev-retrieval.

se7entyse7en · 2019-04-05T14:05:24Z

Rebased and squashed.

Maybe you would like to take another look.

cmd/srcd/cmd/defered_test.go

cmdtests/gitbase_back_comp_test.go

smacker · 2019-04-05T14:14:48Z

cmdtests/gitbase_back_comp_test.go

+func (s *GitbaseBackCompTestSuite) setupWd() {
+	s.T().Helper()
+
+	_, filename, _, _ := runtime.Caller(0)


why do we need runtime.Caller(0) here? Isn't it equal to os.Getwd? If not could you please add a comment. It's not obvious at all.

you're right, it took a couple of seconds even for me to remember the reason...

addressed here.

I still don't get it. We run both integration&regression tests using go test
So why do you need runtime.Caller?
It still looks like:

_, filename, _, _ := runtime.Caller(0) path.Dir(filename)

returns the same results as os.Getwd

And this new comment should belong to setupWd function, not runtime.Caller.
Or am I getting everything wrong?

returns the same results as os.Getwd

It's not path.Dir(filename) actually, but path.Join(path.Dir(filename), "..").

So why do you need runtime.Caller?

Given that regression-core needs the test to be run with the working directory set to the root of the project, I'm exploiting the path of the current file as a reference to build the path of the root of the project. So I'm using runtime.Caller to get the path of gitbase_back_comp_test.go.

And this new comment should belong to setupWd function, not runtime.Caller.

Yup, it actually belongs to the whole function, gonna move it 👍.

cmdtests/gitbase_back_comp_test.go

smacker · 2019-04-05T14:17:05Z

cmdtests/common.go

+}
+
+func AreSQLOutputEqual(s1 string, s2 string) error {
+	return ParseSQLOutput(s1).RequireEqual(ParseSQLOutput(s2))


why do you need to parse results? Is there any problem with simple comparing strings?

AFAIR it was for the rows ordering, but now I guess that we may force the query to include an ORDER BY clause. But I don't know whether in the future we would need to test queries strictly without ORDER BY clauses.

I don't mind to merge it as is. It just seems like an extra code that we can avoid so I mentioned it.
//cc @carlosms

🤔 it's a shame to ask you to remove code that is already in place, but it's true that right now it's a bit convoluted.

If the only thing thing that prevents us from comparing the strings directly is the ORDER BY, I think it's worth it to remove this extra code, to keep the code cleaner and easier to follow.

Oh no! I remember now! But I don't know whether we want to still keep it...

There's (was) another reason. When I developed this at some point it was in-between the replacement of the SQL cli, so there was a version using our custom sql cli and another using mysql cli.

In one case the headers were all capitalized and in the other not, and also if the header is composed by multiple words the separator in one case is the space and in the other was a dash. Moreover, in our sql cli the newline was \n while on mysql is \r\n, but this latter thing is a minimal problem as we can simply replace all the newlines of the buffer.

So if we still want to test versions previous to the sql cli replacement this is needed, but I don't know whether it does make sense to test such older versions.

I we were writing it now I would say we could ignore previous versions. Since it's already done maybe we can keep it, but let's add a comment explaining why this is done. Then later we can remove it if we don't use this to test older versions at all.

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

se7entyse7en · 2019-04-08T10:56:26Z

Rebased, squashed and fixed conflicts.

se7entyse7en · 2019-04-08T13:25:57Z

Are we ready to merge this then?

carlosms · 2019-04-09T14:39:14Z

Let's add this https://github.com/src-d/engine/pull/363#discussion_r272678247 as a comment in the code so it doesn't get lost

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

se7entyse7en · 2019-04-09T17:04:52Z

@carlosms addressed here.

se7entyse7en force-pushed the gitbase-multi-version-testing branch from c8f4510 to 62bf6c6 Compare March 22, 2019 09:34

se7entyse7en requested review from carlosms, dpordomingo and smacker March 22, 2019 09:44

smacker reviewed Mar 22, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

se7entyse7en force-pushed the gitbase-multi-version-testing branch from 4e45352 to d88ff86 Compare March 25, 2019 12:09

carlosms reviewed Mar 25, 2019

View reviewed changes

cmd/test-utils/common.go Outdated Show resolved Hide resolved

carlosms reviewed Mar 25, 2019

View reviewed changes

cmd/srcd/cmd/gitbase_back_comp_test.go Outdated Show resolved Hide resolved

se7entyse7en force-pushed the gitbase-multi-version-testing branch from d88ff86 to 779a53f Compare March 27, 2019 12:37

smacker reviewed Mar 27, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

dpordomingo reviewed Mar 27, 2019

View reviewed changes

cmd/srcd/cmd/gitbase_back_comp_test.go Outdated Show resolved Hide resolved

cmd/srcd/cmd/gitbase_back_comp_test.go Outdated Show resolved Hide resolved

cmd/srcd/cmd/gitbase_back_comp_test.go Outdated Show resolved Hide resolved

se7entyse7en mentioned this pull request Mar 27, 2019

Wait for index creation properly instead of waiting an arbitrary amount of time #378

Closed

dpordomingo approved these changes Mar 28, 2019

View reviewed changes

smacker approved these changes Mar 28, 2019

View reviewed changes

se7entyse7en force-pushed the gitbase-multi-version-testing branch from 2cf3c4d to 91f80a0 Compare April 3, 2019 09:23

smacker reviewed Apr 5, 2019

View reviewed changes

cmd/srcd/cmd/defered_test.go Outdated Show resolved Hide resolved

smacker reviewed Apr 5, 2019

View reviewed changes

cmdtests/gitbase_back_comp_test.go Outdated Show resolved Hide resolved

smacker reviewed Apr 5, 2019

View reviewed changes

cmdtests/gitbase_back_comp_test.go Show resolved Hide resolved

smacker reviewed Apr 5, 2019

View reviewed changes

se7entyse7en force-pushed the gitbase-multi-version-testing branch 3 times, most recently from 60aaedc to 20e4f5a Compare April 8, 2019 10:52

se7entyse7en added 3 commits April 8, 2019 12:53

Extracted running command methods from IntegrationSuite

83d3388

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Added SQLOutputTable

40ca551

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Added RegressionSuite

9d757eb

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

se7entyse7en force-pushed the gitbase-multi-version-testing branch from 20e4f5a to cca2deb Compare April 8, 2019 10:55

se7entyse7en added 7 commits April 8, 2019 12:55

Added regression build tag to cmdtests/common.go

362d260

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Added GitbaseBackCompTestSuite

e57e7ff

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Excluded DeferedTestSuite from regression tests

3554403

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Added a test-regression target in Makefile

68614fe

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Added regression-testing-cache to .gitignore

96a2781

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Removed build tags from defered_test.go

89c57e0

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Updated dependencies

75779c2

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

se7entyse7en force-pushed the gitbase-multi-version-testing branch from cca2deb to 75779c2 Compare April 8, 2019 10:56

carlosms mentioned this pull request Apr 9, 2019

Make use of regression tests #423

Open

Added comment for ParseSQLOutput

acd1537

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

carlosms approved these changes Apr 10, 2019

View reviewed changes

se7entyse7en merged commit 9d11fbc into src-d:master Apr 10, 2019

se7entyse7en deleted the gitbase-multi-version-testing branch April 10, 2019 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gitbase multi version testing #363

Gitbase multi version testing #363

se7entyse7en commented Mar 22, 2019 •

edited

Loading

se7entyse7en commented Mar 22, 2019

se7entyse7en commented Mar 22, 2019 •

edited

Loading

se7entyse7en commented Mar 22, 2019

se7entyse7en commented Mar 22, 2019

smacker commented Mar 22, 2019

carlosms commented Mar 25, 2019

se7entyse7en commented Mar 25, 2019 •

edited

Loading

se7entyse7en commented Mar 26, 2019

smacker commented Mar 26, 2019

se7entyse7en commented Mar 26, 2019

dpordomingo left a comment

se7entyse7en commented Mar 27, 2019

smacker left a comment

se7entyse7en commented Mar 29, 2019

carlosms commented Apr 2, 2019

se7entyse7en commented Apr 2, 2019

smacker commented Apr 2, 2019

se7entyse7en commented Apr 5, 2019

smacker Apr 5, 2019

se7entyse7en Apr 5, 2019

se7entyse7en Apr 5, 2019

smacker Apr 5, 2019

se7entyse7en Apr 5, 2019

smacker Apr 5, 2019

se7entyse7en Apr 5, 2019

smacker Apr 5, 2019

carlosms Apr 5, 2019

se7entyse7en Apr 5, 2019

carlosms Apr 8, 2019

se7entyse7en commented Apr 8, 2019

se7entyse7en commented Apr 8, 2019

carlosms commented Apr 9, 2019

se7entyse7en commented Apr 9, 2019

Gitbase multi version testing #363

Gitbase multi version testing #363

Conversation

se7entyse7en commented Mar 22, 2019 • edited Loading

se7entyse7en commented Mar 22, 2019

se7entyse7en commented Mar 22, 2019 • edited Loading

se7entyse7en commented Mar 22, 2019

se7entyse7en commented Mar 22, 2019

smacker commented Mar 22, 2019

carlosms commented Mar 25, 2019

se7entyse7en commented Mar 25, 2019 • edited Loading

se7entyse7en commented Mar 26, 2019

smacker commented Mar 26, 2019

se7entyse7en commented Mar 26, 2019

dpordomingo left a comment

Choose a reason for hiding this comment

se7entyse7en commented Mar 27, 2019

smacker left a comment

Choose a reason for hiding this comment

se7entyse7en commented Mar 29, 2019

carlosms commented Apr 2, 2019

se7entyse7en commented Apr 2, 2019

smacker commented Apr 2, 2019

se7entyse7en commented Apr 5, 2019

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

se7entyse7en commented Apr 8, 2019

se7entyse7en commented Apr 8, 2019

carlosms commented Apr 9, 2019

se7entyse7en commented Apr 9, 2019

se7entyse7en commented Mar 22, 2019 •

edited

Loading

se7entyse7en commented Mar 22, 2019 •

edited

Loading

se7entyse7en commented Mar 25, 2019 •

edited

Loading