build: parallel integration tests #22
Conversation
Makes all integration tests run in parallel by letting each test use its own database. This also enables the integration tests to be executed in parallel on the emulator, as the emulator does allow multiple operations and transactions to be executed in parallel as long as these are on different databases. Also adds a GitHub Actions configuration file to execute integration tests on the emulator during presubmits. Fixes googleapis#21
google-cla
bot
added
the
cla: yes
| @@ -0,0 +1,717 @@ | |||
| // Copyright 2021 Google Inc. All Rights Reserved. | |||
There was a problem hiding this comment.
I think this should be:
// Copyright 2021 Google LLC
There was a problem hiding this comment.
Same for all other files.
There was a problem hiding this comment.
I'll do that in a separate update to prevent too many merge conflicts. As you mentioned, this should be changed in all the files, including files that are not included in this change.
| keyValue := strings.SplitN(keyValueString, "=", 2) | ||
| if keyValue == nil || len(keyValue) != 2 { | ||
| return nil, fmt.Errorf("invalid connection property: %s", keyValueString) | ||
| } | ||
| params[keyValue[0]] = keyValue[1] | ||
| params[strings.ToLower(keyValue[0])] = keyValue[1] |
There was a problem hiding this comment.
why is this case insensitive?
There was a problem hiding this comment.
This makes sure that any additional parameters in the connection string are case insensitive. So a user does not have to worry whether the parameter name is for example usePlainText or useplaintext (that parameter name would normally be clear for everyone, but for parameters like readonly vs readOnly and autocommit vs autoCommit it's easy to make a mistake).
transaction.go
Outdated
| type retriableStatement interface { | ||
| // retry retries the statement on a new Spanner transaction. The method must | ||
| // return nil if it receives the same result as during the initial attempt, | ||
| // and otherwise ErrAbortedDueToConcurrentModification. |
There was a problem hiding this comment.
otherwise throws an ErrAbortedDueToConcurrentModification error.
There was a problem hiding this comment.
In Go the error is returned and not thrown, so I changed it to and otherwise return the error ErrAbortedDueToConcurrentModification. (Note: 'an ErrAbortedDueToConcurrentModification' is not used here, as this is a global error.)
| return err | ||
| } | ||
|
|
||
| // Commit implements driver.Tx#Commit(). |
There was a problem hiding this comment.
Should we move the second line in the comment to the first line?
// Commit implements driver.Tx#Commit(). It will commit the underlying ...
...There was a problem hiding this comment.
The godoc convention that the first sentence should start with the name of the method, so that would not work with that.
| @@ -85,10 +233,59 @@ func (tx *readWriteTransaction) Rollback() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (tx *readWriteTransaction) Query(ctx context.Context, stmt spanner.Statement) *spanner.RowIterator { | |||
| return tx.rwTx.Query(ctx, stmt) | |||
| func (tx *readWriteTransaction) Query(ctx context.Context, stmt spanner.Statement) rowIterator { | |||
There was a problem hiding this comment.
We should add a comment to Query.
|
@olavloite Can you please rebase other following PRs after this is merged? Thanks. |
Makes all integration tests run in parallel by letting each test use its own database.
This also enables the integration tests to be executed in parallel on the emulator, as
the emulator does allow multiple operations and transactions to be executed in parallel
as long as these are on different databases.
Also adds a GitHub Actions configuration file to execute integration tests on the
emulator during presubmits.
#20 should be reviewed and merged before this.
Fixes #21