build: execute parallel integration tests by olavloite · Pull Request #797 · googleapis/java-spanner-jdbc

olavloite · 2022-04-12T10:24:21Z

Refactors the integration tests to fix a number of problems:

Too much state was kept in the abstract base class for all integration tests. This did not work properly with parameterized tests, as some state leaked from one parameter value (dialect) to another.
The emulator cannot use an existing instance for testing, as it always starts without any existing instances. This is now fixed by forcing tests to used a separate instance for each test class when running on the emulator, and by fixing creation and database cleanup before/after tests.

In addition the integration tests were not executed by Kokoro, as they were skipped in the default test execution. All integration tests are now executed by default.

@ansh0l @mpeddada1
The integration tests are (probably) still failing, but that is because the default test instance (projects/gcloud-devel/instances/spanner-testing-east1) is clogged with old (?) test databases that have not been cleaned up. Some test cases therefore currently fail with an error that they cannot create another database, as the max of 100 databases per instance has been reached. I don't have access to the affected instance, so I cannot clean it up. Hopefully one of you has access to it.
I've verified that the integration tests work on a different instance.

mpeddada1 · 2022-04-19T15:51:49Z

Thanks so much for looking into this @olavloite! This looks good on my end. The Native Image tests are failing because we need to specify some system properties through the maven surefire plugin - this will be addressed in https://github.com/googleapis/java-spanner-jdbc/pull/796/files.

Will let @ansh0l do a final review.

ansh0l

LGTM with few minor nits. Please take a look.

ansh0l · 2022-04-22T07:23:46Z

  ColTimestamp	TIMESTAMP	NOT NULL,
  ColCommitTS	TIMESTAMP	NOT NULL OPTIONS (allow_commit_timestamp=true),
  ColNumeric	NUMERIC		NOT NULL,
+  ColJson		JSON		NOT NULL,


nit: indentation

ansh0l · 2022-04-22T07:24:44Z

@@ -0,0 +1,64 @@
+/*


ansh0l · 2022-04-22T07:25:25Z

+  @Before
+  public void setup() {
+    assumeFalse(
+        "Emulator does not support PostgreSQL",


ansh0l · 2022-04-22T07:26:23Z

+  }
+
  @Override
  public Dialect getDialect() {


should we make getDialect private? setup method seems to be taking care of things now

No, not entirely. Some of the tests are uni-dialect (so only PG or only GoogleSQL), while others are parameterized. The default implementation is uni-dialect and always returns GoogleSQL. This method is then overridden in the test classes that either use only PG or are parameterized. So we cannot make it private.

ansh0l · 2022-04-22T07:28:37Z

+  private Database database;
+
+  @Before
+  public void setup() {


Should the setup method go into ITAbstractJdbcTest class?

I decided to move that away from the abstract base class, as it was one of the main reasons that things were not working properly. The data model that is created by the different test classes is different, and that went wrong for the parameterized test classes, as they need to take the dialect into account as well. So this solves a lot of hassle for those tests at the expense of a little code duplication.

ansh0l · 2022-04-22T07:30:37Z

@@ -27,10 +27,10 @@
 import static org.junit.Assume.assumeFalse;



looks good (with same observations as for setup method before

ansh0l · 2022-04-22T07:31:10Z

-        assertThat(rs.getString("TABLE_CATALOG"), is(equalTo(DEFAULT_CATALOG)));
+        if (!EmulatorSpannerHelper.isUsingEmulator()) {
+          assertThat(rs.next(), is(true));
+          assertThat(rs.getString("TABLE_SCHEM"), is(equalTo("SPANNER_SYS")));


Just checking is TABLE_SCHEM the right intended string?

Good point. Yes, this is intentional. That is one of the strange things about the JDBC definition. They decided to abbreviate one of the column names, and did so by removing only the last letter. Or someone made a typo 25 years ago, and it could never be fixed.... I guess we'll never know.

ansh0l · 2022-04-22T07:32:24Z

+    }
+  }
+
+  private static final String DEFAULT_KEY_FILE = null;


ansh0l · 2022-04-22T07:33:50Z

@olavloite : The resource_exhausted errors should not be occuring any more, if they are, let me / @rajatbhatta know - they are working on a longer term fix for that.

olavloite · 2022-04-22T13:36:05Z

@olavloite : The resource_exhausted errors should not be occuring any more, if they are, let me / @rajatbhatta know - they are working on a longer term fix for that.

Thanks! The last run executed without problems, so hopefully it stays that way.

olavloite added 2 commits April 12, 2022 12:22

build: execute parallel integration tests

1f3feba

fix: remove commented code

b8bc04a

product-auto-label Bot added size: s Pull request size is small. api: spanner Issues related to the googleapis/java-spanner-jdbc API. labels Apr 12, 2022

fix: only create relevant database

d4c2115

product-auto-label Bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Apr 12, 2022

olavloite added 5 commits April 12, 2022 13:31

test: skip PG on emulator

266f5ac

fix: skip table creation if dialect is not supported

6f8aff1

build: make sure db is created before tables

ff95391

fix: ensure own instance on emulator

eae50a2

refactor: cleanup test setup

c976538

product-auto-label Bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Apr 15, 2022

fix: use correct script file

a8900ae

product-auto-label Bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Apr 15, 2022

olavloite added 4 commits April 15, 2022 10:21

fix: metadata test cases

596d59d

fix: reduce fork count + remove error name

b1ded0c

fix: type in sql file name

f0d821d

build: increase fork count to 8

5822006

olavloite marked this pull request as ready for review April 15, 2022 10:44

olavloite requested review from a team April 15, 2022 10:44

olavloite changed the title ~~build: [WIP] execute parallel integration tests~~ build: execute parallel integration tests Apr 15, 2022

olavloite requested review from ansh0l and mpeddada1 April 15, 2022 10:51

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 19, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 19, 2022

fix: return database id as catalog name

ffb3e3f

mpeddada1 approved these changes Apr 20, 2022

View reviewed changes

ansh0l approved these changes Apr 22, 2022

View reviewed changes

chore: address review comments

932d53e

olavloite added the automerge Merge the pull request once unit tests and other checks pass. label Apr 22, 2022

gcf-merge-on-green Bot merged commit 93a0c05 into main Apr 22, 2022

gcf-merge-on-green Bot deleted the execute-parallel-integration-tests branch April 22, 2022 13:52

gcf-merge-on-green Bot removed the automerge Merge the pull request once unit tests and other checks pass. label Apr 22, 2022

		@@ -27,10 +27,10 @@
		import static org.junit.Assume.assumeFalse;

Conversation

olavloite commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpeddada1 commented Apr 19, 2022

Uh oh!

ansh0l left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ansh0l commented Apr 22, 2022

Uh oh!

olavloite commented Apr 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

olavloite commented Apr 12, 2022 •

edited

Loading