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

test: add integration tests using PG native JDBC driver #20

Closed

Conversation

olavloite
Copy link
Collaborator

Adds integration tests that use the native PostgreSQL driver to connect
to PGAdapter, and tweaks the protocol response from PGAdapter to be in
line with what real PostgreSQL does. Notable changes to protocol:

  1. standard_conforming_strings should return 'on' instead of 'true'
  2. Describe Portal should respond with NoData if the portal does not
    contain any columns (i.e. it is an update count)

This change also fixes a wrong assumption in IntermediateStatement that
a zero update count means NO_RESULT. Zero update count means that no
rows were updated/deleted by the UPDATE or DELETE statement.

thiagotnunes and others added 3 commits December 17, 2021 19:40
Adds integration tests that use the native PostgreSQL driver to connect
to PGAdapter, and tweaks the protocol response from PGAdapter to be in
line with what real PostgreSQL does. Notable changes to protocol:
1. standard_conforming_strings should return 'on' instead of 'true'
2. Describe Portal should respond with NoData if the portal does not
   contain any columns (i.e. it is an update count)

This change also fixes a wrong assumption in IntermediateStatement that
a zero update count means NO_RESULT. Zero update count means that no
rows were updated/deleted by the UPDATE or DELETE statement.
@olavloite olavloite marked this pull request as ready for review January 26, 2022 15:32
@olavloite
Copy link
Collaborator Author

cc @voulg @tinaspark @snehashah16 @Vizerai

I don't have access to the main repository, so I cannot add any of you as reviewers (and I have to create the PR from a fork). PTAL.

The tests are failing, as GitHub Actions cannot download the required dependencies, and does not have access to the credentials needed for integration tests.

Comment on lines -59 to -64
this.executed = false;
this.exception = null;
this.resultType = null;
this.hasMoreData = false;
this.statementResult = null;
this.updateCount = null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, as this is a no-op (it just sets a number of properties to their default values)


/**
* Extracts what type of result exists within the statement. In JDBC a statement update count is
* positive if it is an update statement, 0 if there is no result, or negative if there are
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A zero update count does not mean that there is no result. It means that zero rows were updated/deleted.

@@ -102,7 +102,7 @@ public static void sendStartupMessage(
new ParameterStatusResponse(output, "client_encoding".getBytes(), "utf8".getBytes()).send();
new ParameterStatusResponse(output, "DateStyle".getBytes(), "ISO".getBytes()).send();
new ParameterStatusResponse(output, "IntervalStyle".getBytes(), "iso_8601".getBytes()).send();
new ParameterStatusResponse(output, "standard_conforming_strings".getBytes(), "true".getBytes())
new ParameterStatusResponse(output, "standard_conforming_strings".getBytes(), "on".getBytes())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch (this.statement.getResultType()) {
case UPDATE_COUNT:
case NO_RESULT:
new NoDataResponse(this.outputStream).send();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostgreSQL returns NoData as as response to a Describe Portal for an update count or other statement that does not return a result set.

Comment on lines -114 to -116
// Utility class for setting up test connection.
private RemoteSpannerHelper spannerHelper;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, as it was never used

public SpannerOptions spannerOptions() {
return options;
public Spanner getSpanner() {
if (spanner == null) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a single shared Spanner instance instead of returning SpannerOptions. The Spanner instance is closed when the test environment is cleaned up.

@@ -145,9 +145,9 @@ public void testBasicNoResultStatement() throws Exception {
Mockito.verify(statement, Mockito.times(1))
.execute("UPDATE users SET name = someName WHERE id = -1");
Assert.assertFalse(intermediateStatement.containsResultSet());
Assert.assertEquals((int) intermediateStatement.getUpdateCount(), -2);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An actual update statement will never return -2 in real life.

Vizerai and others added 7 commits January 27, 2022 02:54
…grade-jdbc-to-2.5.7-pg-SNAPSHOT

deps: upgrade jdbc to 2.5.7-pg-SNAPSHOT
Adds integration tests that use the native PostgreSQL driver to connect
to PGAdapter, and tweaks the protocol response from PGAdapter to be in
line with what real PostgreSQL does. Notable changes to protocol:
1. standard_conforming_strings should return 'on' instead of 'true'
2. Describe Portal should respond with NoData if the portal does not
   contain any columns (i.e. it is an update count)

This change also fixes a wrong assumption in IntermediateStatement that
a zero update count means NO_RESULT. Zero update count means that no
rows were updated/deleted by the UPDATE or DELETE statement.
@olavloite
Copy link
Collaborator Author

Closing, replaced by #23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants