-
Notifications
You must be signed in to change notification settings - Fork 26
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
Gitbase multi version testing #363
Merged
se7entyse7en
merged 11 commits into
src-d:master
from
se7entyse7en:gitbase-multi-version-testing
Apr 10, 2019
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
83d3388
Extracted running command methods from IntegrationSuite
se7entyse7en 40ca551
Added SQLOutputTable
se7entyse7en 9d757eb
Added RegressionSuite
se7entyse7en 362d260
Added regression build tag to cmdtests/common.go
se7entyse7en e57e7ff
Added GitbaseBackCompTestSuite
se7entyse7en 3554403
Excluded DeferedTestSuite from regression tests
se7entyse7en 68614fe
Added a test-regression target in Makefile
se7entyse7en 96a2781
Added regression-testing-cache to .gitignore
se7entyse7en 89c57e0
Removed build tags from defered_test.go
se7entyse7en 75779c2
Updated dependencies
se7entyse7en acd1537
Added comment for ParseSQLOutput
se7entyse7en File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,3 +27,4 @@ main | |
# CI | ||
.ci/ | ||
build/ | ||
regression-testing-cache/ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
// +build integration regression | ||
|
||
package cmdtests | ||
|
||
import ( | ||
"path" | ||
"runtime" | ||
"time" | ||
|
||
"gotest.tools/icmd" | ||
) | ||
|
||
type Commander struct { | ||
bin string | ||
} | ||
|
||
func NewCommander(bin string) *Commander { | ||
return &Commander{bin: bin} | ||
} | ||
|
||
func (s *Commander) Bin() string { | ||
return s.bin | ||
} | ||
|
||
func (s *Commander) RunCmd(cmd string, args []string, cmdOperators ...icmd.CmdOp) *icmd.Result { | ||
args = append([]string{cmd}, args...) | ||
return icmd.RunCmd(icmd.Command(s.bin, args...), cmdOperators...) | ||
} | ||
|
||
func (s *Commander) RunCommand(cmd string, args ...string) *icmd.Result { | ||
return s.RunCmd(cmd, args) | ||
} | ||
|
||
func (s *Commander) StartCommand(cmd string, args []string, cmdOperators ...icmd.CmdOp) *icmd.Result { | ||
args = append([]string{cmd}, args...) | ||
return icmd.StartCmd(icmd.Command(s.bin, args...)) | ||
} | ||
|
||
func (s *Commander) Wait(timeout time.Duration, r *icmd.Result) *icmd.Result { | ||
return icmd.WaitOnCmd(timeout, r) | ||
} | ||
|
||
// RunInit runs srcd init with workdir and custom config for integration tests | ||
func (s *Commander) RunInit(workdir string) *icmd.Result { | ||
return s.RunInitWithTimeout(workdir, 0) | ||
} | ||
|
||
// RunInitWithTimeout runs srcd init with workdir and custom config for integration tests with timeout | ||
func (s *Commander) RunInitWithTimeout(workdir string, timeout time.Duration) *icmd.Result { | ||
_, filename, _, _ := runtime.Caller(0) | ||
configFile := path.Join(path.Dir(filename), "..", "integration-testing-config.yaml") | ||
return s.RunCmd("init", []string{workdir, "--config", configFile}, icmd.WithTimeout(timeout)) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
why do you need to parse results? Is there any problem with simple comparing strings?
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.
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 withoutORDER BY
clauses.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 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
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.
🤔 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.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.
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.
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 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.