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:
[email protected]
