-
Notifications
You must be signed in to change notification settings - Fork 63
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 support for the source command #128
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM, with some questions and suggestions.
@@ -606,8 +660,8 @@ func (t *tester) concurrentExecute(querys []query, wg *sync.WaitGroup, errOccure | |||
} | |||
} | |||
|
|||
func (t *tester) loadQueries() ([]query, error) { | |||
data, err := os.ReadFile(t.testFileName()) | |||
func (t *tester) loadQueries(fileName string) ([]query, error) { |
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.
Should we have a new tester
for a new --source
'd file instead of using the upper level tester
with a different filename argument?
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 think so, the included files are still part of the same test.
t/source.test
Outdated
@@ -0,0 +1,3 @@ | |||
--echo first line | |||
--source include/hello.inc |
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.
Should we also test multi-level --source
?
I.e.:
t/source.test:
--echo first line
--source include/hello.inc
--echo last line
include/hello.inc:
--echo Hello
--source include/world.inc
--echo !
include/world.inc:
--echo World
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 think that's needed
// Process the queries in the file | ||
includedQueries, err := t.loadQueries(fileName) | ||
if err != nil { | ||
return testCnt, errors.Annotate(err, fmt.Sprintf("error loading queries from %s", fileName)) |
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.
Will this lead to a full 'stack trace'? I.e. so you can follow from the originating file's line, to the current --source
'd file and line?
Is this testable? Maybe with a unit test?
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.
dvaneeden@dve-carbon:~/dev/pingcap/mysql-tester$ chmod 000 include/hello3.inc
dvaneeden@dve-carbon:~/dev/pingcap/mysql-tester$ ./mysql-tester source
ERRO[0000] run test [source] err: error loading queries from include/hello3.inc: open include/hello3.inc: permission denied
ERRO[0000] 1 tests failed
ERRO[0000] run test [source] err: error loading queries from include/hello3.inc: open include/hello3.inc: permission denied
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.
We could add the location:
dvaneeden@dve-carbon:~/dev/pingcap/mysql-tester$ ./mysql-tester source
ERRO[0000] run test [source] err: error loading queries from include/hello3.inc, line ./t/source.test:11: open include/hello3.inc: permission denied
ERRO[0000] 1 tests failed
ERRO[0000] run test [source] err: error loading queries from include/hello3.inc, line ./t/source.test:11: open include/hello3.inc: permission denied
dvaneeden@dve-carbon:~/dev/pingcap/mysql-tester$ git diff
diff --git a/src/main.go b/src/main.go
index 5e09363..3808f49 100644
--- a/src/main.go
+++ b/src/main.go
@@ -559,7 +559,7 @@ func (t *tester) runQueries(queries []query) (int, error) {
// Process the queries in the file
includedQueries, err := t.loadQueries(fileName)
if err != nil {
- return testCnt, errors.Annotate(err, fmt.Sprintf("error loading queries from %s", fileName))
+ return testCnt, errors.Annotate(err, fmt.Sprintf("error loading queries from %s, line %s", fileName, q.location()))
}
includeCnt, err := t.runQueries(includedQueries)
if err != nil {
// Disable the `--source` command by default to avoid breaking existing tests | ||
disableSource = true | ||
|
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.
Should there be a program argument to set it enabled by default, similar to --check-error
argument, so it would work with test files that contain --source
'd files that does not exists or have not yet been tested successfully?
<file>:<line>
instead.--source
, but never actually ran the included files or tested for their presence./etc/password
, etc)Related:
source file_name
)Existing tests
This disables
--source
by default and emits a warning like this:So when porting tests one has to add
--enable-source
to the top of the file. It would probably good to change this in the future to make it enabled by default and require--disable-source
to disable it for files where it breaks (or remove /fix the--source
lines)