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.

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

dveeden
Copy link
Collaborator

@dveeden dveeden commented Jul 26, 2024

dvaneeden@dve-carbon:~/dev/pingcap/mysql-tester$ cat t/source.test 
--echo first line
--source include/hello.inc
--echo last line
dvaneeden@dve-carbon:~/dev/pingcap/mysql-tester$ cat include/hello.inc 
--echo Hello from the included file
dvaneeden@dve-carbon:~/dev/pingcap/mysql-tester$ cat r/source.result 
first line
Hello from the included file
last line
  • A lot of places that only reported the line number, these should now report <file>:<line> instead.
  • This improves compatibility with the official MySQL test runner from Oracle
  • This has the potential to break existing tests that were defined with --source, but never actually ran the included files or tested for their presence.
  • This tries to restrict including random files (/etc/password, etc)
  • This does not prevent against including files in include files and causing endless loops etc

Related:

Existing tests

This disables --source by default and emits a warning like this:

WARN[0000] source command disabled, add '--enable_source' to your file to enable  line="./t/source.test:8"

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)

@dveeden dveeden requested review from bb7133 and mjonss July 26, 2024 08:54
Copy link
Collaborator

@mjonss mjonss left a 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) {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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
Copy link
Collaborator

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

Copy link
Collaborator Author

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))
Copy link
Collaborator

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?

Copy link
Collaborator Author

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 

Copy link
Collaborator Author

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 {

@dveeden dveeden marked this pull request as draft July 29, 2024 09:34
mjonss
mjonss previously approved these changes Jul 29, 2024
@dveeden dveeden marked this pull request as ready for review July 30, 2024 07:04
Comment on lines +54 to +56
// Disable the `--source` command by default to avoid breaking existing tests
disableSource = true

Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants