cmd/smithtest: add sqlsmith auto tester and github filer #40453
Conversation
|
This change is |
|
@knz can you review the CLI interactive change? |
There was a problem hiding this comment.
Yeah so I reviewed this and I strongly recommend against this approach.
- the interactive boolean does imply a few other things besides just printing the intro text, and you probably don't want those things to occur
- printing the connection URL on the message shown to all users of demo will erroneously suggest that cockroach demo starts a "server" that they can connect to from other tools, or even suggest that they must use an external tool to use it. I find this coutner-productive to the kind of UX we want to advertise for
cockroach demo, and it may increase our support burden in unexpected ways.
Instead, please consider issuing the following SQL query via the command's stdin instead: select value from crdb_internal.node_runtime_info where component='DB' and field='URL'; which will give you that URL too.
Reviewed 3 of 5 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @knz, @rafiss, and @solongordon)
|
In fact I'd go a step further and argue that hijacking I'd instead recommend building an executable program that sets up a cockroachdb test server like any unit test and prints the connection URL on its stderr, and have that custom executable driven by your sqlsmith command. They would live next to each other in the same package and this way you don't create a mandate for every contributor to |
There was a problem hiding this comment.
i reviewed the internals of the new command, bu i defer to knz about COCKROACH_FORCE_INTERACTIVE
Reviewed 1 of 1 files at r2.
Reviewable status:
pkg/cmd/smithtest/main.go, line 68 at r2 (raw file):
rand.Seed(timeutil.Now().UnixNano()) // Prepopulate seen issues from GitHub.
nit: populating seen issues could be extracted to a separate function
pkg/cmd/smithtest/main.go, line 71 at r2 (raw file):
var opts github.SearchOptions for { results, _, err := setup.github.Search.Issues(ctx, "is:issue is:open label:C-bug label:O-sqlsmith", &opts)
i haven't used this github client library, so i might be missing something. how is this search being restricted to only the cockroachdb repo?
pkg/cmd/smithtest/main.go, line 98 at r2 (raw file):
// Setup contains initialization and configuration for running smithers. type Setup struct {
nit: the name Setup doesn't really seem descriptive to me, but i also just may not know the naming conventions for stuff like this in go. perhaps a name like WorkerSetup or even WorkerPool? also, does the type need to be exported?
pkg/cmd/smithtest/main.go, line 228 at r2 (raw file):
// it takes for a single reduction run. lock syncutil.RWMutex seen = map[string]bool{}
nit: it could be helpful to declare these earlier in the file, since i notice they are used earlier. and perhaps add a comment to seen
pkg/cmd/smithtest/main.go, line 289 at r2 (raw file):
RawQuery: query.Encode(), } const max = 8000
just curious: do most issue bodies with stacktraces fit into this limit?
|
question from #40594 (comment)
What about you use the same technique used in |
|
Using I started working on a standalone program that does all of this, and it's just an annoying amount of work to get around something that I believe is a theoretical problem. I already have this thing working with a minimal change to cockroach demo. I don't care if some other user breaks smithtest by changing the behavior of cockroach demo, I'll just fix it.
Sure, I'm fine with this. I'd rather deal with any possible UX fallout than write a new program that's like mostly already this. What I have here already works perfectly with only a tiny amount of weirdness added to cockroach demo. The amount of work required to make this work correctly without using cockroach demo has a higher cost than dealing with theoretical fallout. |
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/smithtest/main.go, line 71 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i haven't used this
githubclient library, so i might be missing something. how is this search being restricted to only the cockroachdb repo?
Fixed.
pkg/cmd/smithtest/main.go, line 98 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: the name
Setupdoesn't really seem descriptive to me, but i also just may not know the naming conventions for stuff like this in go. perhaps a name likeWorkerSetupor evenWorkerPool? also, does the type need to be exported?
Renamed. I just wanted it uppercase to indicate its importance. Doesn't matter for commands.
pkg/cmd/smithtest/main.go, line 289 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
just curious: do most issue bodies with stacktraces fit into this limit?
Yes. I only found a few that don't, mostly caused by large queries that weren't very reduceable.
|
Updated to use a pty package and |
|
PTAL |
There was a problem hiding this comment.
Reviewed 3 of 5 files at r3.
Reviewable status:
pkg/cmd/smithtest/main.go, line 156 at r3 (raw file):
"--port", "0", "--http-port", "0", "--insecure",
Add --logtostderr to this and remove the pty thing altogether. The log will go to stderr and you can pick the details up from there.
Bonus is that this way you'll also get stack traces and crash reports for fatal errors, not just panics (you'd only get panics otherwise).
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/smithtest/main.go, line 156 at r3 (raw file):
Previously, knz (kena) wrote…
Add
--logtostderrto this and remove theptything altogether. The log will go to stderr and you can pick the details up from there.Bonus is that this way you'll also get stack traces and crash reports for fatal errors, not just panics (you'd only get panics otherwise).
Done. Thanks.
There was a problem hiding this comment.
But otherwise thanks!
Reviewed 3 of 3 files at r4.
Reviewable status:
pkg/cmd/smithtest/main.go, line 141 at r4 (raw file):
// run is a single sqlsmith worker. It starts a new sqlsmither and cockroach // demo instance. If an error is found it reduces and submits the issue. If
comment needs adjustment. "and in-memory single-node cluster"
pkg/cmd/smithtest/main.go, line 147 at r4 (raw file):
// nil, since they are expected. Something unexpected would be like the // initialization SQL was unable to run. func (s WorkerSetup) run(ctx context.Context, rnd *rand.Rand) error {
My preference would be to split this large functions into small(er) sub-functions. Your call.
smithtest is a CLI program for automating sqlsmith testing. It does normal sqlsmith but via cockroach start-single-node, so found panics don't also take down the test itself. Found errors are de-duplicated using existing github issues. The reducer is used to minimize test repros. A new tab is opened to github with most of the issue data filled in. Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/smithtest/main.go, line 141 at r4 (raw file):
Previously, knz (kena) wrote…
comment needs adjustment. "and in-memory single-node cluster"
Done.
|
bors r+ |
40453: cmd/smithtest: add sqlsmith auto tester and github filer r=mjibson a=mjibson smithtest is a CLI program for automating sqlsmith testing. It does normal sqlsmith but via cockroach start-single-node, so found panics don't also take down the test itself. Found errors are de-duplicated using existing github issues. The reducer is used to minimize test repros. A new tab is opened to github with most of the issue data filled in. Use a pty package to enable interactive mode in cockroach, which prints the sql address to stdout, enabling us to scrape it and connect there. Release note: None Release justification: Category 1: Non-production code changes Co-authored-by: Matt Jibson <[email protected]>
Build succeeded |
smithtest is a CLI program for automating sqlsmith testing. It does normal
sqlsmith but via cockroach start-single-node, so found panics don't also
take down the test itself. Found errors are de-duplicated using existing
github issues. The reducer is used to minimize test repros. A new tab
is opened to github with most of the issue data filled in.
Use a pty package to enable interactive mode in cockroach, which prints
the sql address to stdout, enabling us to scrape it and connect there.
Release note: None
Release justification: Category 1: Non-production code changes