cli: add framework for writing dump tests#22403
cli: add framework for writing dump tests#22403justinj wants to merge 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
madelynnblue
left a comment
There was a problem hiding this comment.
I love this. One change I want to see first: after running the dump command, re-run it with -e and dump it again and verify that both dumps are the same. This is important to make sure things roundtrip correctly, which will detect problems in the dump output itself, which we've had before, for example when table names weren't correctly escaped.
pkg/cli/testdata/dump_test
Outdated
|
|
||
| % name reference_cycle | ||
| % command dump d t | ||
| -- This tests dumping in the presence of cycles. This used to crash before |
There was a problem hiding this comment.
I would suggest using # instead of -- for comments, like we do in the logic tests.
|
(I haven't forgotten about this PR, just have more time-sensitive things to handle at the moment, I will come back to it) |
I quite frequently find myself having to add new tests and functionality to dump. While the existing setup for writing tests was quite good, and easy to add new tests, I often found myself wanting to be able to `rewrite-results-in-testfiles` it (especially due to whitespace issues), and not write my tests in a string literal, so my attempt at changing this is a somewhat radical one - a logic test-esque file format that's composed of a schema and an expected dump output. I think this general format of * parameters incl. test name * input * output could be quite useful in general and we might be able to extract some kind of general framework for it in the future if we decide to go that route. Release note: None.
fb46801 to
5d42464
Compare
|
Ok, I think this is ready for another look! (Again, feel free to ignore if you're time-pressured at the moment, this is the opposite of time-sensitive) Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending. pkg/cli/testdata/dump_test, line 46 at r1 (raw file): Previously, jordanlewis (Jordan Lewis) wrote…
Done, though I'm kind of torn here, it is more consistent than our logic tests, but the character Comments from Reviewable |
|
Looks awesome; will review more when I have time (probably after code freeze). |
|
superseded by #24653 |
I quite frequently find myself having to add new tests and functionality
to dump. While the existing setup for writing tests was quite good, and
easy to add new tests, I often found myself wanting to be able to
rewrite-results-in-testfilesit (especially due to whitespace issues),and not write my tests in a string literal, so my attempt at changing
this is a somewhat radical one - a logic test-esque file format that's
composed of a schema and an expected dump output.
Feel free to reject this PR for any of the following reasons, my
feelings won't be hurt:
This PR is still missing a number of things, however:
that at this point, more interested in thoughts on the general approach.
I think this general format of
could be quite useful in general and we might be able to extract some
kind of general framework for it in the future if we decide to go that
route.