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

Support --let #49

Merged
merged 11 commits into from
Nov 15, 2023
Merged

Support --let #49

merged 11 commits into from
Nov 15, 2023

Conversation

dveeden
Copy link
Collaborator

@dveeden dveeden commented Jan 11, 2022

Issue: ref #110

  • Support setting variables based on a constant --let $a=5
  • Support setting variables based on a query: --let $b=`SELECT VERSION()`
  • Support echoing back variables --echo $a and $b

See also: https://dev.mysql.com/doc/dev/mysql-server/latest/PAGE_MYSQL_TEST_COMMANDS.html

$ cat t/let.test 
--echo $SHELL
--let $a= 5
--echo a is $a
--let $b=`SELECT VERSION()`
--echo b is $b
$ ./mysql-tester --record let
INFO[0000] recording tests: [let]                       
WARN[0000] Create new db&{0 0xc000126000 0 {0 0} [0xc000290000] map[] 0 1 0xc00012a000 false map[0xc000290000:map[0xc000290000:true]] map[] 0 0 0 0 <nil> 0 0 0 0 0x49d880} 
./t/let.test: ok! 0 test cases passed, take time 0.000539828 s
INFO[0000] run test [let] ok                            

Great, All tests passed
$ cat r/let.result 
/bin/bash
a is 5
b is 5.7.25-TiDB-v5.5.0-alpha-79-g1d20bbfeb-dirty

@dveeden
Copy link
Collaborator Author

dveeden commented Jan 11, 2022

cc @morgo @mjonss

morgo
morgo previously approved these changes Jan 17, 2022
mjonss
mjonss previously approved these changes Jan 17, 2022
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 + some suggestions.

Also should we start to add more tests? Like in t/*.test and r/*.result files?

src/main.go Show resolved Hide resolved
src/main.go Outdated Show resolved Hide resolved
Co-authored-by: Mattias Jonsson <mjonss@users.noreply.github.com>
@dveeden dveeden dismissed stale reviews from mjonss and morgo via 021d2f6 January 18, 2022 17:39
@dveeden
Copy link
Collaborator Author

dveeden commented Jan 18, 2022

/cc @bb7133

@dveeden dveeden requested a review from bb7133 November 8, 2022 10:58
src/main.go Outdated Show resolved Hide resolved
@dveeden dveeden requested review from bb7133 and mjonss March 7, 2023 09:38
@Defined2014 Defined2014 mentioned this pull request Nov 10, 2023
9 tasks
@dveeden dveeden self-assigned this Nov 10, 2023
@dveeden
Copy link
Collaborator Author

dveeden commented Nov 13, 2023

let.test:

--echo $SHELL
--let $a= 5
--echo a is $a
--let $b=`SELECT "foobar"`
--echo b is $b
--let $abc123= "foo bar baz"
--echo abc123 is $abc123
--let $def_456= "aaa bbb ccc"
--echo $def_456

let.result:

/bin/bash
a is 5
b is foobar
abc123 is "foo bar baz"
"aaa bbb ccc"

@dveeden
Copy link
Collaborator Author

dveeden commented Nov 13, 2023

PTAL @mjonss

mjonss
mjonss previously approved these changes Nov 13, 2023
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, but please also add some tests.

I'm approving it, but I think we can extend this a bit further to be more compatible with the let command of mysqltest client. Like handling backtick quoting the same.

Should we also make a feature request for --expr?

src/main.go Show resolved Hide resolved
@dveeden
Copy link
Collaborator Author

dveeden commented Nov 13, 2023

LGTM, but please also add some tests.

I've added a test for Q_LET in 4ab1f13

We could add tests like #49 (comment) in this repo or rely on this being used in other repos.

I'm approving it, but I think we can extend this a bit further to be more compatible with the let command of mysqltest client. Like handling backtick quoting the same.

Yes, we could do that, but this does most of what most tests need. I think we can extend this in another PR later when needed.

Should we also make a feature request for --expr?

I'm not sure if there is any demand for that. Maybe add a note about this in #110 ?

@dveeden
Copy link
Collaborator Author

dveeden commented Nov 13, 2023

PTAL @bb7133

@dveeden dveeden removed their assignment Nov 13, 2023
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 closed this Nov 15, 2023
@bb7133 bb7133 reopened this Nov 15, 2023
@bb7133 bb7133 merged commit fc3c1e6 into pingcap:master Nov 15, 2023
1 check passed
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.

None yet

5 participants