sql/logictest: Fixes logic test tabular data output #127773
Conversation
|
This change is |
a443127
to
b945eb6
Compare
b945eb6
to
c2c2d67
Compare
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
7b47930
to
dba487b
Compare
There was a problem hiding this comment.
nice
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @kyle-a-wong, and @xinhaoz)
pkg/sql/logictest/logic_test.go line 36 at r1 (raw file):
func TestFormatting(t *testing.T) { defer leaktest.AfterTest(t)() RunLogicTest(t, TestServerArgs{}, 0, testfile)
do you know why we had to introduce a new test file like this and a new explicit RunLogicTest call? it seems like this should just work if we do mv pkg/sql/logictest/testdata/testdata pkg/sql/logictest/testdata/logic_test/formatting and then run ./dev gen logictest
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/logic_test.go line 36 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
do you know why we had to introduce a new test file like this and a new explicit
RunLogicTestcall? it seems like this should just work if we domv pkg/sql/logictest/testdata/testdata pkg/sql/logictest/testdata/logic_test/formattingand then run./dev gen logictest
Well the main reason was was because I didn't know about this logictest generation lol. But also I'm not sure it belongs in testdata/logic_test. The intention behind this test is the test that the formatted values generated and output by logic tests is working as intended. Its not really trying to test any logic in the DB and doesn't need to be tested different config.
If my assumptions are wrong or you think it should be tested with different configs / logic tests, I can move it here.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/logic_test.go line 36 at r1 (raw file):
Previously, kyle-a-wong (Kyle Wong) wrote…
Well the main reason was was because I didn't know about this logictest generation lol. But also I'm not sure it belongs in
testdata/logic_test. The intention behind this test is the test that the formatted values generated and output by logic tests is working as intended. Its not really trying to test any logic in the DB and doesn't need to be tested different config.If my assumptions are wrong or you think it should be tested with different configs / logic tests, I can move it here.
i think i'd still prefer it to be in the main testdata directory. that way, it's less likely for the test to be forgotten if the framework changes in the future. since we don't need to run it under all configs, you can add this at the top of the file:
The file can also have a comment saying that it's just testing the logictest framework's formatting logic.
logictest generated test data formats query results in a in a tabular format using tabwriter. when a query result contains `\n` characters, it formats it such that stays within the same tab cell on output. This works well when there is a single row result, but when there are multiple rows with data that contains `\n` characters, the intput parser doesn't know, nor can it tell, if a cell is multi-lined or not, erroneously reporting that data expected data and actual data don't match. To Fix, multi-row query responses will now replace `\n` characters with `\\n` to ensure tests pass while being well formatted. Fixes: cockroachdb#127785 Release note: None
dba487b
to
3b6b5f1
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/logic_test.go line 36 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i think i'd still prefer it to be in the main testdata directory. that way, it's less likely for the test to be forgotten if the framework changes in the future. since we don't need to run it under all configs, you can add this at the top of the file:
The file can also have a comment saying that it's just testing the logictest framework's formatting logic.
Done.
|
Thanks for the reviews and comments! bors r+ |
logictest generated test data formats query results in a in a tabular format using tabwriter. when a query result contains
\ncharacters, it formats it such that stays within the same tab cell on output. This works well when there is a single row result, but when there are multiple rows with data that contains\ncharacters, the input parser doesn't know, nor can it tell, if a cell is multi-lined or not, erroneously reporting that data expected data and actual data don't match.To Fix, multi-row query responses will now replace
\ncharacters with\\nto ensure tests pass while being well formatted.Fixes: #127785
Release note: None