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

sql: changes for allowing tests in sql package to set up a server #6473

Merged
merged 4 commits into from May 4, 2016

Conversation

@RaduBerinde
Copy link
Member

RaduBerinde commented May 3, 2016

We currently run most sql tests from the sql_test package to avoid a circular dependency on server. This means that everything the tests need must be exported from sql. The distributed sql package will have the same problem.

The commits in this PR provide a solution to this problem, with a "sample" proof-of-concept test. This test will be removed when we add a real test that uses this infrastructure, or we move over (some of) the existing tests to sql.


This change is Reviewable

@nvanbenschoten

This comment has been minimized.

Copy link
Member

nvanbenschoten commented May 3, 2016

:lgtm:


Reviewed 4 of 4 files at r1, 14 of 14 files at r2, 4 of 4 files at r3, 13 of 13 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
This doesn't seem to fit with Go's interface naming convention. Perhaps SchemaChangersClearer.


sql/main_test.go, line 243 [r3] (raw file):
So sql_test.TestMain is called even when testing within the sql package?


Comments from Reviewable

@RaduBerinde

This comment has been minimized.

Copy link
Member Author

RaduBerinde commented May 3, 2016

TFTR!


Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
Heh I had used Manipulator and renamed it to Callback at @andreimatei's request. I wanted something a bit more generic than Clearer in case we add more functions in the future. Andrei, what do you think?


sql/main_test.go, line 243 [r3] (raw file):
Yes - there can only be one TestMain across the two packages. Also all init() functions in all the tests (sql and sql_test) are called before any tests run (regardless which tests are run).


Comments from Reviewable

@andreimatei

This comment has been minimized.

Copy link
Member

andreimatei commented May 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
maybe my suggestion was bad :)

But I stand by the spirit - maybe SchemaChangersCallbackArg. I was thinking the point of this interface is not actually to express a number of actions that can be taken on the schema changers, but rather to actually represent the schema changers (so users would downcast, see below). If you think we want to make the hook accessible to tests outside of the sql package, maybe Manipulator is good after all.

How about we make the callback's param be of typeinterface{}, we leave this TestingSchemaChangerCollection as a struct, and we do the downcast to it in the tests. And soon when tests move to the sql package, we get rid of the struct and have the tests downcast directly to the internal schemaChangerCollection.


Comments from Reviewable

@RaduBerinde

This comment has been minimized.

Copy link
Member Author

RaduBerinde commented May 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
Where do we downcast? The callbacks just call the ClearSchemaChangers() function on the interface.. How is an interface{}+downcast preferable to that?


Comments from Reviewable

@andreimatei

This comment has been minimized.

Copy link
Member

andreimatei commented May 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
well it'd arguably be preferable because you wouldn't have to introduce this interface, and keep adding methods to it for other things the tests might want to do in the future. The whole point of moving tests to the sql package is that they get access to internals and we don't need to worry about interfaces likes this.
And we could package the downcast so the test doesn't have to worry about it. Like, we could have a method in sql/testutils:

func InstallSchemaChangerHook(params *shim.TestServerParams, callback func (sc *SchemaChangerCollection)) {
  params.TestingKnobs.(*..).ExecutorTestingKnobs.SchemaChangerHook = func (arg interface{}) {
  callback(arg.(*SchemaChangerCollection))
})
}

I think we're gonna end up with these sort of functions since we chose to not put TestServer and all the knobs in the sql package.


Comments from Reviewable

@RaduBerinde

This comment has been minimized.

Copy link
Member Author

RaduBerinde commented May 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
I think all this is pretty moot as we won't need this at all if we move the test in sql. I had done this cleanup back when I first tried to move the full definition of the knobs in base and then continued working on top of it. I can revert this part if you prefer..


Comments from Reviewable

@andreimatei

This comment has been minimized.

Copy link
Member

andreimatei commented May 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
well so how exactly is it gonna go away when tests are in sql? You'll need the downcast,no?


Comments from Reviewable

@RaduBerinde

This comment has been minimized.

Copy link
Member Author

RaduBerinde commented May 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
We can get rid of this type, we won't need to export anything anymore.. We can just pass the *schemaChangerCollection directly. Isn't this what the TODO is talking about?


Comments from Reviewable

@andreimatei

This comment has been minimized.

Copy link
Member

andreimatei commented May 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
well that'd be ideal, but if the callback that needs to reference it (they type of the hook function) is out of the sql, how's that gonna work?


Comments from Reviewable

@RaduBerinde

This comment has been minimized.

Copy link
Member Author

RaduBerinde commented May 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
I thought only sql tests use this?


Comments from Reviewable

@RaduBerinde

This comment has been minimized.

Copy link
Member Author

RaduBerinde commented May 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
Wait, how does your proposal work for uses outside of sql? If we export the schemaChangerCollection type, why do we need an interface{} at all? (instead of just passing a *SchemaChangerCollection?)


Comments from Reviewable

@andreimatei

This comment has been minimized.

Copy link
Member

andreimatei commented May 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
I was just thinking of tests in sql. Sorry, I was confused. I forgot we're putting the various testing knobs in their respective packages. Forget everything I've said; let's get rid of this interface.


Comments from Reviewable

@RaduBerinde

This comment has been minimized.

Copy link
Member Author

RaduBerinde commented May 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/executor.go, line 169 [r1] (raw file):
Ok reverting this commit and then we will be able to remove this when we move the tests


Comments from Reviewable

@RaduBerinde RaduBerinde force-pushed the RaduBerinde:testserver-shim branch from f62491b to 76edfdb May 3, 2016
RaduBerinde added 4 commits May 4, 2016
We move `TestingKnobs` to `base` and make the members use a generic interface.
This will allow us to pass `TestingKnobs` to a server without depending on `sql`
or `storage`.
Remove the PGUrl dependency on server by passing the address directly.
This change adds infrastructure for a TestServerShim which can be used to
interface with TestServer without depending on `server`.

The central trick is that `TestMain` can run from the `sql_main` package
which has access to `server` and can initialize the shim. The initialization
could also happen as part of an `init()` function in `sql_main`. Kudos to
@andreimatei and @tamird for coming up with this idea.

This will allow us to run tests from the `sql` package and access `sql`
internals, as shown by a (temporary) proof-of-concept test. The same will apply
to `dist_sql` stuff when it moves to a separate package.
"declaration of res shadows declaration at sql/group.go:327"
@RaduBerinde RaduBerinde force-pushed the RaduBerinde:testserver-shim branch from 76edfdb to f32b090 May 4, 2016
@andreimatei

This comment has been minimized.

Copy link
Member

andreimatei commented May 4, 2016

:lgtm:


Review status: 6 of 28 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

@RaduBerinde RaduBerinde merged commit 6076500 into cockroachdb:master May 4, 2016
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
code-review/reviewable Review complete: 1 of 0 LGTMs obtained (and 1 stale)
Details
licence/cla Contributor License Agreement is signed.
Details
@RaduBerinde RaduBerinde deleted the RaduBerinde:testserver-shim branch May 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.