Run integration tests in-process #385

daviddrysdale · 2017-02-16T16:41:55Z

Should be rather faster (e.g. 1.5s for CT test, 9s for log test)

gdbelvin · 2017-02-16T17:20:21Z

integration/ct_integration_test.go

 	if err != nil {
 		t.Fatalf("Failed to read log config: %v", err)
 	}

+	if err := runParallelTests(cfgs, *httpServersFlag, *testDir, *mmdFlag); err != nil {


Recommend using t.Parallel() and the main test runner.

Not sure I follow how to do that -- t.Parallel appears to run different tests in parallel, whereas this is running multiple instances of the same test in parallel.

(BTW, the Travis builds include a variant with -race set, so we can hopefully catch concurrency issues.)

If you'd like to run the same test multiple times under different conditions, please use Go's "Subtests" feature. Each subtest can be run in parallel to speed things up using t.Parallel()
https://golang.org/pkg/testing/#hdr-Subtests_and_Sub_benchmarks

Aha, that's the bit I was missing -- thanks for the pointer.

gdbelvin · 2017-02-16T17:23:16Z

server/log_operation_manager.go

@@ -130,6 +130,11 @@ func (l LogOperationManager) getLogsAndExecutePass(ctx context.Context) bool {
 	return false
 }

+// OperationSingle performs a single pass of the manager.
+func (l LogOperationManager) OperationSingle() {


I like this :-)

gdbelvin · 2017-02-16T17:23:47Z

server/log_operation_manager.go

+func (l LogOperationManager) OperationSingle() {
+	l.getLogsAndExecutePass(l.context.ctx)
+}
+
 // OperationLoop starts the manager working. It continues until told to exit.
 // TODO(Martin2112): No mechanism for error reporting etc., this is OK for v1 but needs work
 func (l LogOperationManager) OperationLoop() {


Have Operation loop use OperationSingle

OperationLoop needs the quit flag that getLogsAndExecutePass returns, but that's not relevant for OperationSingle, so I think it's clearer as-is.

gdbelvin · 2017-02-16T17:25:16Z

testonly/integration/clientserver.go

@@ -74,14 +83,13 @@ func listen() (string, net.Listener, error) {

 // getTestDB drops and recreates the test database.
 // Returns a database connection to the test database.
-func getTestDB(testID string) (*sql.DB, error) {
+func getTestDB(testID string) (*sql.DB, string, error) {


We were trying to isolate the db connection string away from other functions and just return a proper sql.DB. Is there a reason why we can't maintain this abstraction?

Good point, will do.

gdbelvin · 2017-02-16T17:25:57Z

testonly/integration/clientserver.go

 	if err != nil {
 		return nil, err
 	}

-	return &LogEnv{


Prefer creating the returned structure at the end of the function rather than setting individual parameters throughout function. Helps with code clarity

sync.WaitGroup cannot be copied, so creating the structure at the end doesn't work for that, and it seemed better to be consistent.

Scratch that, I can just store a pointer to the one true sync.WaitGroup

gdbelvin · 2017-02-16T17:27:16Z

testonly/integration/clientserver.go

 		sequencerWindow)
-	sequencerTask := server.NewLogOperationManagerForTest(ctx, registry,
-		batchSize, sleep, timeSource, sequencerManager)
+	if numSequencers == 0 {


If you'd like to test multiple sequencers, convert env.Sequencer into a slice.

That's not how it works -- the existing code is somewhat confusing because it gives the name sequencerTask to a LogOperationManager object (of which there is only one). The LogOperationManager will then spawn goroutines for the given count of sequencers.

gdbelvin · 2017-02-16T17:28:17Z

testonly/integration/clientserver.go

+		env.Sequencer = server.NewLogOperationManagerForTest(ctx, registry,
+			batchSize, sleepBetweenRuns, timeSource, env.LogOperation)
+	} else {
+		// Start a live sequencer in a goroutine.


We don't want to launch any goroutines here because that introduces concurrency into tests.
Separate out the launching of sequencers into a separate function that individual tests can call.

Heh, the boat has sailed on avoiding concurrency. The existing code already launches one goroutine at the top level (go grpcServer.Serve(lis)), and even with the test one-shot LogOperationManager, there will be a bunch of goroutines spawned under the covers (see SequencerManager.ExecutePass).

gdbelvin · 2017-02-16T17:31:04Z

testonly/integration/clientserver.go

+}
+
+// LaunchCT adds a running CT personality to the LogEnv test environment.
+func (env *LogEnv) LaunchCT(cfgs []ct.LogConfig) error {


Please move CT integration tests to a separate file. Perhaps to the example CT directory to enforce separation of personality specific test functions.

I'll move it into a separate file, but given that the directory is testonly/integration I think the code belongs in this package for the time being.

Added a new CTLogEnv on top of LogEnv, in testonly/integration/ct.go

gdbelvin · 2017-02-16T17:32:00Z

testonly/integration/clientserver.go

+		}
+	}
+	// Wait long enough for a signer run to happen, so empty tree gets signed.
+	time.Sleep(2 * sleepBetweenRuns)


Use a OS.Signal rather than sleep to avoid concurrency problems.

Actually, I think I can just delete this, it was left over from before I created OperationSingle.

Also, does OS.Signal do anything clever with threads/goroutines? Because UNIX signals and threads traditionally don't mix well...

daviddrysdale · 2017-02-17T13:22:54Z

PTAL

gdbelvin · 2017-02-20T10:54:52Z

integration/ct_integration_test.go

+	mmd := 120 * time.Second
+	// Run a container for the parallel sub-tests, so that we wait until they
+	// all complete before terminating the test environment.
+	t.Run("container", func(t *testing.T) {


Consider using testing.Main to establish common setup and teardown code for each test.

Ah, but there isn't common setup/teardown code -- it's different for different tests.

gdbelvin

Thanks for these changes.

Consider also moving the semantics around the graceful shutdown of a LogOperationManager inside LogOperationManager.

gdbelvin · 2017-02-20T10:58:40Z

integration/ct_integration_local.cfg

@@ -0,0 +1,26 @@
+[


This log config file appears to only be used in the ct_integration test.
Consider moving this to be a const inside the ct_integration_test.

The reason is that paths in Go are considered to be relative to the main or test package that is running.

Done, but I don't understand the reason you mention (the expanded []ct.LogConfig slice will also contain a bunch of path names, relative to the test).

Putting the paths inside the test file makes it clear that this config data is test only.
Just a bit of self documentation, a speedup (avoids a file.Open) and compile time checking of the data structure.

gdbelvin · 2017-02-20T11:05:30Z

integration/ct_integration_test.go

-		wg.Add(1)
-		go func(c ctfe.LogConfig) {
-			defer wg.Done()
+	for _, cfg := range cfgs {


if cfgs weren't loaded from a file, this could be a proper table run test.

for _, test := range struct { // test vars } { { //test case }, } { // test code }

gdbelvin · 2017-02-20T11:10:43Z

testonly/integration/clientserver.go

@@ -76,7 +77,6 @@ func listen() (string, net.Listener, error) {
 // Returns a database connection to the test database.
 func getTestDB(testID string) (*sql.DB, error) {
 	var testDBURI = fmt.Sprintf("root@tcp(127.0.0.1:3306)/log_unittest_%v", testID)
-	builtin.MySQLURIFlag = &testDBURI


Thanks for fixing this hack

gdbelvin · 2017-02-20T11:18:19Z

testonly/integration/clientserver.go

 		return nil, err
 	}
-	go grpcServer.Serve(lis)
+	wg.Add(1)


GracefulStop waits for the server to complete active RPCs.
This should eliminate the need to add this to the wait group, which I would love to avoid creating here.

log.Close() already calls grpcServer.GracefulStop(), so I'm not sure what you're after here.

I think a wait group is absolutely essential when we're starting separate goroutines -- for example, to ensure that any shutdown failure shows up as a hang/test timeout rather than an obscure database locking issue.

If I'm understanding you properly, you're saying that there should always be some kind of thread.Join logic for each go routine all the way up to the main thread. - Agreed.

I'm just saying that this wait group / thread.Join logic should be contained inside server.GracefulStop so we don't need to recreate it here.

gdbelvin · 2017-02-20T11:20:39Z

testonly/integration/clientserver.go

+		sequencerTask = server.NewLogOperationManager(ctx2, registry,
+			batchSize, numSequencers, sleepBetweenRuns, timesource, sequencerManager)
+		wg.Add(1)
+		go func(wg *sync.WaitGroup, om *server.LogOperationManager) {


Rather than adding the sequencer to a wait group, is there a way we could create a GracefulStop to the log operation manager as well?

It seems to me that the management of what sequencer runs are active etc, and how to do a proper shutdown should be internal to the log operation manager.

Probably a good idea, but rather beyond the scope of this PR.

Thanks. If you don't mind, could you create and merge #391 first?
That would be a great help.

gdbelvin · 2017-02-20T11:21:28Z

testonly/integration/clientserver.go

+	sequencerManager := server.NewSequencerManager(keyManager, registry, sequencerWindow)
+	var wg sync.WaitGroup
+	var sequencerTask *server.LogOperationManager
+	var cancel context.CancelFunc


Consider moving the cancel semantics and wait group inside the LogOperationManager

See comment below.

gdbelvin · 2017-02-20T11:22:35Z

testonly/integration/clientserver.go

 	}, nil
 }

 // Close shuts down the server.
 func (env *LogEnv) Close() {
+	if env.sequencerCancel != nil {


Consider replacing with env.Sequencer.GracefulStop() which should block until the sequencer has been stopped.

This is a factory function to build an Extension that uses the provided database and KeyManager, rather than relying on global command-line flags.

Track running goroutines with a sync.WaitGroup to ensure proper completion. Add LogOperationManager.OperationSingle() for running a single pass, rather than relying on the 'test' instance just running once.

gdbelvin · 2017-02-20T15:33:29Z

Please address #391 before merging this PR.

daviddrysdale · 2017-02-20T15:50:06Z

Rebased to fix merge conflict (now injecting a KeyManager to NewExtensionRegistry as well as a databse) and also folded changes so far into granular commits.

daviddrysdale · 2017-02-21T08:59:38Z

The whole point of raising a separate issue (#391) was that changing LogOperationManager's behaviour is out of scope for this change. So no, I'm not going to address it before merging this PR.

Martin2112

I think #391 should be addressed separately as it will be easier to review.

Martin2112 · 2017-02-21T09:16:20Z

integration/log_integration_test.go

+
+func TestInProcessLogIntegration(t *testing.T) {
+	ctx := context.Background()
+	env, err := integration.NewLogEnv(ctx, 2, "TestInProcessLogIntegration")


Pull out magic number '2' as a const? Also possibly 6962.

Builds on LogEnv by also configuring a collection of CT logs and running a CT log personality in a goroutine.

…m 589b12611..55c1d0c85 8cc3a55af Add custom options to allow more control of swagger/openapi output (google#145) b0be3cdef runtime: fix chunk encoding 82b83c781 protoc-gen-swagger optional SourceCodeInfo 1fd8ba6a5 Fix logic handling primitive wrapper in URL params b2423da79 runtime: use r.Context() (google#473) c323909dd Add Handler method to pass in a client (google#454) ac41185c3 Fallback to JSON name when matching URL parameter. (google#450) 8bec008bd fix 2 typos in Registry.SetPrefix's comment de5a00fcc Reference Gulp by a more complete path 185dda2d4 Fix build. 824b9a716 Test with Go 1.9. f2862b476 Memoise calls to fullyQualifiedNameToSwaggerName to speed it up for large registries (google#421) 1a03ca3ba Update DO NOT EDIT template. (google#434) a5c7982c0 Update Swagger Codegen from 2.1.6 to 2.2.2 (google#415) d64f5319e ISSUE#405: customize the error return (google#409) c6f7a5ac6 improve {incoming,outgoing}HeaderMatcher logic (google#408) 2a40dd795 Return if runtime.AnnotateContext gave error (google#403) 47a11d786 jsonpb: update tests to reflect new jsonpb behavior (google#401) f6f92fcd9 Reference import grpc Status to suppress unused errors. (google#387) 979be44d9 fixes package name override doesn't work (google#277) b1e4aed16 Skip unreferenced messages in definitions. (google#371) ca4c8d6af ci: regen with current protoc-gen-go (google#385) 7195ea445 Use status package for error and introduce WithProtoErrorHandler option (google#378) 4539fc575 Add response headers from grpc server (google#374) 597c8c358 support allow_delete_body for protoc-gen-grpc-gateway (google#318) 55d0969c0 Use canonical header form in default header matcher. (google#369) 893772d22 Extend ServeMux to allow user configurable header forwarding. git-subtree-dir: vendor/github.com/grpc-ecosystem/grpc-gateway git-subtree-split: 55c1d0c857e5c6cadb0ee292f6cc36621cd5ea8c

googlebot added cla: yes labels Feb 16, 2017

daviddrysdale assigned gdbelvin Feb 16, 2017

gdbelvin reviewed Feb 16, 2017

View reviewed changes

gdbelvin suggested changes Feb 16, 2017

View reviewed changes

gdbelvin reviewed Feb 20, 2017

View reviewed changes

gdbelvin suggested changes Feb 20, 2017

View reviewed changes

daviddrysdale added 3 commits February 20, 2017 14:31

extension: add NewExtensionRegistry(db, km)

e3f827c

This is a factory function to build an Extension that uses the provided database and KeyManager, rather than relying on global command-line flags.

testonly/integration: inject context

80d47b8

testonly/integration: option for live signer

fa34b14

Track running goroutines with a sync.WaitGroup to ensure proper completion. Add LogOperationManager.OperationSingle() for running a single pass, rather than relying on the 'test' instance just running once.

daviddrysdale force-pushed the goroutines branch from 1e3fa29 to 20a94bc Compare February 20, 2017 15:48

daviddrysdale assigned Martin2112 Feb 21, 2017

Martin2112 approved these changes Feb 21, 2017

View reviewed changes

daviddrysdale added 4 commits February 21, 2017 09:35

integration: in-process log test

1be478c

testonly/integration: add CTLogEnv

ffd026e

Builds on LogEnv by also configuring a collection of CT logs and running a CT log personality in a goroutine.

integration/ct: in-process CT test

7260b65

integration/ct: use t.Run() for parallel tests

63bcf64

daviddrysdale force-pushed the goroutines branch from 20a94bc to 63bcf64 Compare February 21, 2017 09:39

daviddrysdale merged commit 06b7fa5 into google:master Feb 21, 2017

daviddrysdale deleted the goroutines branch February 21, 2017 09:54

Run integration tests in-process #385

Run integration tests in-process #385

Conversation

daviddrysdale commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddrysdale Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddrysdale Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddrysdale commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdbelvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddrysdale Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

gdbelvin Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdbelvin commented Feb 20, 2017

daviddrysdale commented Feb 20, 2017

daviddrysdale commented Feb 21, 2017

Martin2112 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddrysdale Feb 16, 2017 •

edited

Loading

daviddrysdale Feb 16, 2017 •

edited

Loading

daviddrysdale Feb 20, 2017 •

edited

Loading

gdbelvin Feb 20, 2017 •

edited

Loading