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

executor: Improve the aesthetics of code review #6137

Merged
merged 29 commits into from
Mar 31, 2018

Conversation

kangxiaoning
Copy link
Contributor

Improve the aesthetics of code review in executor/grant_test.go.

@sre-bot
Copy link
Contributor

sre-bot commented Mar 24, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@CLAassistant
Copy link

CLAassistant commented Mar 24, 2018

CLA assistant check
All committers have signed the CLA.

tk.MustExec("CREATE USER 'dduser'@'%' IDENTIFIED by '123456';")
tk.MustExec("GRANT ALL PRIVILEGES ON `dddb_%`.* TO 'dduser'@'%';")
tk.MustExec("GRANT ALL PRIVILEGES ON `dddb_%`.`te%` to 'dduser'@'%';")
tk.MustExec("CREATE USER 'dduser'@`%` IDENTIFIED by '123456';")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You modified it wrong. You should modify the quotation marks for the SQL statement instead of the quotation marks for %.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@winkyao @yangwenmai I don't understand why changing the backquote to single quotes would report the following error. So keep the original backquote.

FAIL: grant_test.go:177: testSuite.TestIssue2456

grant_test.go:180:
    tk.MustExec(`GRANT ALL PRIVILEGES ON 'dddb_%'.* TO 'dduser'@'%';`)
/Users/marong/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:160:
    tk.c.Assert(err, check.IsNil, check.Commentf("sql:%s, %v, error stack %v", sql, args, errors.ErrorStack(err)))
... value *errors.Err = &errors.Err{message:"", cause:(*errors.errorString)(0xc4201c2d40), previous:(*errors.Err)(0xc4205d2730), file:"/Users/marong/go/src/github.com/pingcap/tidb/util/testkit/testkit.go", line:134} ("line 1 column 32 near \".* TO 'dduser'@'%';\" (total length 51)")
... sql:GRANT ALL PRIVILEGES ON 'dddb_%'.* TO 'dduser'@'%';, [], error stack line 1 column 32 near ".* TO 'dduser'@'%';" (total length 51)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just keep the original code is ok:

tk.MustExec("CREATE USER 'dduser'@'%' IDENTIFIED by '123456';")
tk.MustExec("GRANT ALL PRIVILEGES ON `dddb_%`.* TO 'dduser'@'%';")
tk.MustExec("GRANT ALL PRIVILEGES ON `dddb_%`.`te%` to 'dduser'@'%';")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@winkyao executor/grant_test.go have been changed to original, thx.

@shenli shenli added the contribution This PR is from a community contributor. label Mar 24, 2018
@ngaut
Copy link
Member

ngaut commented Mar 27, 2018

PTAL @yangwenmai

@ngaut
Copy link
Member

ngaut commented Mar 29, 2018

LGTM

@ngaut
Copy link
Member

ngaut commented Mar 29, 2018

/run-all-tests

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zz-jason zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label Mar 29, 2018
@zz-jason
Copy link
Member

/run-unit-test

@coocood coocood merged commit 67f920a into pingcap:master Mar 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants