-
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 actual check of --error command not just that an error exists #52
Conversation
I took a look at how many of the existing tests will break: unfortunately it's most :( I wonder if there is a way to make this behavior transitional? For example the test would need to include the following: --strict_error_check TRUE After we've fixed all the tests we remove this flag and have it on all the time. The other option is we divide all the tests by a few people and let them break for a few days while we fix them. |
Here is the list of failures (58 of 121 fail):
|
@mjonss I discussed this with others today, and @Alkaagr81 had a suggestion on how we can retrofit this to the current tests a little better. Her proposal was:
The warnings will be annoying to start with, but we can clean them up and then validate that they match correctly. |
Thanks for the comments! I have updated the patch with a |
Thanks! I've confirmed it works as expected:
It looks like you might need to import the tidb error code constants into your PR? i.e.
Otherwise LGTM |
@mjonss could you fix the conflicts? |
Makefile
Outdated
@@ -15,3 +20,5 @@ clean: | |||
go clean -i ./... | |||
rm -rf mysql-tester | |||
|
|||
gen_perror: |
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.
gen_perror: | |
build_gen_perror: |
And then maybe something like this:
gen_perror: build_gen_perror
./gen_perror
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.
Also gen_perror
(the current one) should be added to .PHONY:
If more performance is needed you might want to use a WaitGroup. The fix: diff --git a/generate_perror/main.go b/generate_perror/main.go
index 6c0b1f7..f37abc6 100644
--- a/generate_perror/main.go
+++ b/generate_perror/main.go
@@ -140,6 +140,7 @@ func main() {
checkNewErr(m[2], i, NameToNum)
}
}
+ cmd.Wait()
}
if maxError >= 1000 {
fmt.Printf("\r") |
Do we need both versions? Can't we only use
|
Some examples I've seen:
So allowing both makes less existing tests to be modified. |
Could we use something like this? //go:generate ../gen_perror |
@@ -1055,6 +1131,16 @@ func main() { | |||
flag.Parse() | |||
tests := flag.Args() | |||
startTime := time.Now() | |||
if ll := os.Getenv("LOG_LEVEL"); ll != "" { |
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.
Can't we use a flag like -d
/-debug
or something similar? I think that makes it easier to see from the usage output how to set the loglevel. We could use both if CI or something else depends on this.
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.
there is already a '-log-level' command line option/flag, does that work?
Yes, but I don't think it is worth doing it in this PR, I wrote that code long time ago just to avoid a shell script oneliner (see the comment) and I don't expect the generator to be run very often. |
) | ||
|
||
func init() { | ||
flag.StringVar(&tidbCodePath, "path", "../tidb", "Path to TiDB source code root directory.") |
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.
Does this mean we need to rely on tidb?
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.
Yes, in the same way as it relies on perror
(a binary from the mysql distribution, to map error numbers to error code names).
If TiDB would have a similar program/binary/utility, to map error codes to names, we could use that instead of reading the source code.
But since we are generating the perror.go file, with the mapping, and that is checked into the mysql-tester repository, it only depends on perror
and tidb
repository when it needs to be updated. So it does not have any direct dependencies, only needs the dependencies when new error codes needs to be added.
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.
Got it.
Co-authored-by: Daniël van Eeden <github@myname.nl>
6 | ||
1 SELECT; | ||
Error 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use line 1 column 1 near "1 SELECT;" | ||
2 SELECT; |
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 this SQL doesn't have any error message? Seems error no begin with 0
will ignore the error msg. Does it same as mysql?
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.
Checked MySQL-Server, they are same.
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
Currently the
--error <ERROR CODE>[,...]
only checks if there is an error, not that the error matches, this PR adds such check.Also since most tests uses error names like
ER_NO_SUCH_TABLE
a map was needed from name to number, which was generated withperror
.