You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by GitBox <gi...@apache.org> on 2021/01/05 14:38:58 UTC
[GitHub] [metamodel] wcedmisten opened a new pull request #245: METAMODEL-1231 PostgreSQL integration test errors
wcedmisten opened a new pull request #245:
URL: https://github.com/apache/metamodel/pull/245
Fixes the following error when running the JDBC integration test:
```
ERROR 17:38:56 JdbcUtils - Could not execute batch: INSERT INTO "public"."my_table" ("person name",age) VALUES ('John Doe','42'): Batch entry 0 INSERT INTO "public"."my_table" ("person name",age) VALUES ('John Doe','42') was aborted: ERROR: column
"age" is of type integer but expression is of type character varying
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [metamodel] arjansh commented on a change in pull request #245: METAMODEL-1231 PostgreSQL integration test errors
Posted by GitBox <gi...@apache.org>.
arjansh commented on a change in pull request #245:
URL: https://github.com/apache/metamodel/pull/245#discussion_r553808771
##########
File path: jdbc/src/test/java/org/apache/metamodel/jdbc/integrationtests/PostgresqlTest.java
##########
@@ -736,7 +736,7 @@ public void run(UpdateCallback cb) {
"Column[name=age,columnNumber=2,type=INTEGER,nullable=true,nativeType=int4,columnSize=10]",
table.getColumnByName("age").toString());
- cb.insertInto(table).value("person name", "John Doe").value("age", "42").execute();
+ cb.insertInto(table).value("person name", "John Doe").value("age", 42).execute();
Review comment:
Notice that this test case is called "testInsertFailureForStringValueForIntegerColumn". As you can see on line 745 of the file, it contains this assertion:
```
assertEquals(
"java.sql.BatchUpdateException: Batch entry 0 INSERT INTO \"public\".\"my_table\" (\"person name\",age) VALUES ('John Doe','42') was aborted: ERROR: column \"age\" is of type integer but expression is of type character varying Hint: You will need to rewrite or cast the expression. Position: 64 Call getNextException to see other errors in the batch.",
message);
```
So the test actually expects the insert statement to fail.
Having said that, there are a number of things which can be improved for this test case (and probably the complete PostgreslTest class).
First of all, as you demonstrate here, if you change the test, by changing "42" to 42, the insert statement will not fail and the test will not fail. Which makes this an unreliable test case. I'm not sure if this is easy to fix using JUnit 3, which this test class is using, but if the test class was refactored to use JUnit 4, an annotation like this could be added to the test case:
```
@Test(expected = Exception.class)
```
or preferably the `ExpectedException` rule can be used by adding this to the class:
```
@Rule
public ExpectedException exceptionRule = ExpectedException.none();
```
and this to the method:
```
exceptionRule.expect(Exception.class);
exceptionRule.expectMessage("java.sql.BatchUpdateException: Batch entry 0 INSERT INTO \"public\".\"my_table\" (\"person name\",age) VALUES ('John Doe','42') was aborted: ERROR: column \"age\" is of type integer but expression is of type character varying Hint: You will need to rewrite or cast the expression. Position: 64 Call getNextException to see other errors in the batch.");
```
Second of all, even though my first proposal would probably make the test case more reliable, you'd probably still get the error messages in the build log. And while these are expected in this case, they still are ugly and something which you'd rather not see when doing a normal build. It might be able to get rid of these by adding this dependency (with the "test" scope) to the module's pom.xml:
```
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-nop</artifactId>
<scope>test</scope>
</dependency>
```
This dependency introduces a "no operation" logger which is only used in the "test" scope, so during tests nothing is logged using the logger. This is an effective manner to get rid of error messages in your build log during the test phase when you test things which are expected to fail and log errors. Getting this to work might however take some more work than simple adding this dependency to the pom.xml.
So in short, I won't merge this pull request, but I agree that the current implementation of this test case can be improved.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [metamodel] wcedmisten commented on a change in pull request #245: METAMODEL-1231 PostgreSQL integration test errors
Posted by GitBox <gi...@apache.org>.
wcedmisten commented on a change in pull request #245:
URL: https://github.com/apache/metamodel/pull/245#discussion_r554208074
##########
File path: jdbc/src/test/java/org/apache/metamodel/jdbc/integrationtests/PostgresqlTest.java
##########
@@ -736,7 +736,7 @@ public void run(UpdateCallback cb) {
"Column[name=age,columnNumber=2,type=INTEGER,nullable=true,nativeType=int4,columnSize=10]",
table.getColumnByName("age").toString());
- cb.insertInto(table).value("person name", "John Doe").value("age", "42").execute();
+ cb.insertInto(table).value("person name", "John Doe").value("age", 42).execute();
Review comment:
Thanks for the detailed feedback and recommendations! I should have read the test more closely.
I have followed some of your recommendations for this PR, and would love to have the current changes reviewed.
I initially updated the JDBC integration test syntax to Junit4, but then I realized that this won't entirely solve the problem.
This integration test uses the `finally` block to clean up test data from the database (dropping the table). As far as I know, there is no way to have similar cleanup for this one test function, when using Junit4's `@Test(expected = Exception.class)` or `exceptionRule.expect(Exception.class);` A potential workaround might be to override the `tearDown()` function to clean up after each test, but this would apply to every test, and might be too generic. At the very least, it would require the other tests to be refactored.
Instead of this approach, I added a `fail()` call right after the code which is intended to throw an exception, so that if this exception is not thrown, the test will fail. This can be tested by changing:
```
cb.insertInto(table).value("person name", "John Doe").value("age", "42").execute();
```
on `jdbc/src/test/java/org/apache/metamodel/jdbc/integrationtests/PostgresqlTest.java:761` to
```
cb.insertInto(table).value("person name", "John Doe").value("age", 42).execute();
```
This would mean that an exception is no longer thrown, and the test should fail.
I had already updated the syntax to Junit4, but let me know if you would like me to roll this back, because it's not strictly required for this change set.
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [metamodel] asfgit merged pull request #245: METAMODEL-1231 PostgreSQL integration test errors
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #245:
URL: https://github.com/apache/metamodel/pull/245
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [metamodel] arjansh commented on a change in pull request #245: METAMODEL-1231 PostgreSQL integration test errors
Posted by GitBox <gi...@apache.org>.
arjansh commented on a change in pull request #245:
URL: https://github.com/apache/metamodel/pull/245#discussion_r553808771
##########
File path: jdbc/src/test/java/org/apache/metamodel/jdbc/integrationtests/PostgresqlTest.java
##########
@@ -736,7 +736,7 @@ public void run(UpdateCallback cb) {
"Column[name=age,columnNumber=2,type=INTEGER,nullable=true,nativeType=int4,columnSize=10]",
table.getColumnByName("age").toString());
- cb.insertInto(table).value("person name", "John Doe").value("age", "42").execute();
+ cb.insertInto(table).value("person name", "John Doe").value("age", 42).execute();
Review comment:
Notice that this test case is called "testInsertFailureForStringValueForIntegerColumn". As you can see on line 745 of the file, it contains this assertion:
```
assertEquals(
"java.sql.BatchUpdateException: Batch entry 0 INSERT INTO \"public\".\"my_table\" (\"person name\",age) VALUES ('John Doe','42') was aborted: ERROR: column \"age\" is of type integer but expression is of type character varying Hint: You will need to rewrite or cast the expression. Position: 64 Call getNextException to see other errors in the batch.",
message);
```
So the test actually expects the insert statement to fail.
Having said that, there are a number of things which can be improved for this test case (and probably the complete PostgreslTest class).
First of all, as you demonstrate here, if you change the test, by changing "42" to 42, the insert statement will not fail and the test will not fail. Which makes this an unreliable test case. I'm not sure if this is easy to fix using JUnit 3, which this test class is using, but if the test class was refactored to use JUnit 4, an annotation like this could be added to the test case:
```
@Test(expected = Exception.class)
```
or preferably the `ExpectedException` rule can be used by adding this to the class:
```
@Rule
public ExpectedException exceptionRule = ExpectedException.none();
```
and this to the method:
```
exceptionRule.expect(Exception.class);
exceptionRule.expectMessage("java.sql.BatchUpdateException: Batch entry 0 INSERT INTO \"public\".\"my_table\" (\"person name\",age) VALUES ('John Doe','42') was aborted: ERROR: column \"age\" is of type integer but expression is of type character varying Hint: You will need to rewrite or cast the expression. Position: 64 Call getNextException to see other errors in the batch.");
```
Second of all, even though my first proposal would probably make the test case more reliable, you'd probably still get the error messages in the build log. And while these are expected in this case, they still are ugly and something which you'd rather not see when doing a normal build. It might be able to get rid of these by adding this dependency (with the "test" scope) to the module's pom.xml:
```
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-nop</artifactId>
<scope>test</scope>
</dependency>
```
This dependency introduces a "no operation" logger which is only used in the "test" scope, so during tests nothing is logged using the logger. This is an effective manner to get rid of error messages in your build log during the test phase when you test things which are expected to fail and log errors. Getting this to work might however take some more work than simple adding this dependency to the pom.xml.
So in short, I won't merge this pull request, but I agree that the current implementation of this test case can be improved.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [metamodel] arjansh commented on a change in pull request #245: METAMODEL-1231 PostgreSQL integration test errors
Posted by GitBox <gi...@apache.org>.
arjansh commented on a change in pull request #245:
URL: https://github.com/apache/metamodel/pull/245#discussion_r557250383
##########
File path: jdbc/src/test/java/org/apache/metamodel/jdbc/integrationtests/PostgresqlTest.java
##########
@@ -92,13 +99,14 @@ public void testCompositePrimaryKeyCreation() throws Exception {
JdbcTestTemplates.compositeKeyCreation(getDataContext(), "metamodel_test_composite_keys");
}
+ @Test
public void testInterpretationOfNull() throws Exception {
if (!isConfigured()) {
return;
}
JdbcTestTemplates.interpretationOfNulls(getConnection());
}
-
+
Review comment:
Can you remove these trailing white-spaces?
##########
File path: jdbc/src/test/java/org/apache/metamodel/jdbc/integrationtests/DB2Test.java
##########
@@ -26,6 +26,8 @@
import org.apache.metamodel.query.Query;
import org.apache.metamodel.schema.Schema;
import org.apache.metamodel.schema.Table;
+import org.junit.Test;
+import static org.junit.Assert.*;
Review comment:
Static imports are usually put at the top of the import list. (this comment applies to all java files where you added static imports). If your using an IDE, check if it contains an "Organize Imports" option. If so, please apply it to the files you changed. It should move the static imports to the top (and maybe reorganize some other imports too).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [metamodel] wcedmisten commented on a change in pull request #245: METAMODEL-1231 PostgreSQL integration test errors
Posted by GitBox <gi...@apache.org>.
wcedmisten commented on a change in pull request #245:
URL: https://github.com/apache/metamodel/pull/245#discussion_r554208074
##########
File path: jdbc/src/test/java/org/apache/metamodel/jdbc/integrationtests/PostgresqlTest.java
##########
@@ -736,7 +736,7 @@ public void run(UpdateCallback cb) {
"Column[name=age,columnNumber=2,type=INTEGER,nullable=true,nativeType=int4,columnSize=10]",
table.getColumnByName("age").toString());
- cb.insertInto(table).value("person name", "John Doe").value("age", "42").execute();
+ cb.insertInto(table).value("person name", "John Doe").value("age", 42).execute();
Review comment:
Thanks for the detailed feedback and recommendations! I should have read the test more closely.
I have followed some of your recommendations for this PR, and would love to have the current changes reviewed.
I initially updated the JDBC integration test syntax to Junit4, but then I realized that this won't entirely solve the problem.
This integration test uses the `finally` block to clean up test data from the database (dropping the table). As far as I know, there is no way to have similar cleanup for this one test function, when using Junit4's `@Test(expected = Exception.class)` or `exceptionRule.expect(Exception.class);` A potential workaround might be to override the `tearDown()` function to clean up after each test, but this would apply to every test, and might be too generic. At the very least, it would require the other tests to be refactored.
Instead of this approach, I added a `fail()` call right after the code which is intended to throw an exception, so that if this exception is not thrown, the test will fail. This can be tested by changing:
```
cb.insertInto(table).value("person name", "John Doe").value("age", "42").execute();
```
on `jdbc/src/test/java/org/apache/metamodel/jdbc/integrationtests/PostgresqlTest.java:761` to
```
cb.insertInto(table).value("person name", "John Doe").value("age", 42).execute();
```
This would mean that an exception is no longer thrown, and the test should fail.
I had already updated the syntax to Junit4, but let me know if you would like me to roll this back, because it's not strictly required for this change set.
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [metamodel] wcedmisten commented on a change in pull request #245: METAMODEL-1231 PostgreSQL integration test errors
Posted by GitBox <gi...@apache.org>.
wcedmisten commented on a change in pull request #245:
URL: https://github.com/apache/metamodel/pull/245#discussion_r557499239
##########
File path: jdbc/src/test/java/org/apache/metamodel/jdbc/integrationtests/DB2Test.java
##########
@@ -26,6 +26,8 @@
import org.apache.metamodel.query.Query;
import org.apache.metamodel.schema.Schema;
import org.apache.metamodel.schema.Table;
+import org.junit.Test;
+import static org.junit.Assert.*;
Review comment:
My IDE (IntelliJ Ultimate 2020) seems to automatically move the static import to the bottom, so I just manually moved the static ones to the top, for consistency with this codebase. The "organize import" also pulled in some new imports, so let me know if I should go back and remove them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org