[GitHub] [metamodel] arjansh commented on a change in pull request #245: METAMODEL-1231 PostgreSQL integration test errors

[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