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:
us...@infra.apache.org
