You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openjpa.apache.org by GitBox <gi...@apache.org> on 2021/04/10 14:57:02 UTC
[GitHub] [openjpa] eolivelli opened a new pull request #79: HerdDB profile for integration tests
eolivelli opened a new pull request #79:
URL: https://github.com/apache/openjpa/pull/79
This is a draft, I am going to add a profile that adds HerdDB 0.20.0 dependency and start HerdDB server with docker
--
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] [openjpa] eolivelli commented on pull request #79: HerdDB profile for integration tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #79:
URL: https://github.com/apache/openjpa/pull/79#issuecomment-817149717
@struberg @rmannibucau FYI
I am just starting...
I hope we can have something useful to test the release
--
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] [openjpa] struberg commented on a change in pull request #79: HerdDB profile for integration tests
Posted by GitBox <gi...@apache.org>.
struberg commented on a change in pull request #79:
URL: https://github.com/apache/openjpa/pull/79#discussion_r611063825
##########
File path: pom.xml
##########
@@ -1102,6 +1102,58 @@
</properties>
</profile>
+ <!-- Profile for testing with HerdDB -->
+ <profile>
+ <id>test-herddb-docker</id>
+ <activation>
+ <property>
+ <name>test-herddb-docker</name>
+ </property>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>org.herddb</groupId>
+ <artifactId>herddb-jdbc</artifactId>
+ <version>0.22.0</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+ <properties>
+ <connection.driver.name>herddb.jdbc.Driver</connection.driver.name>
Review comment:
For me docker is also fine. The benefit of a docker image is that you drop it and have a very well defined initial status.
--
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] [openjpa] rmannibucau commented on a change in pull request #79: HerdDB profile for integration tests
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #79:
URL: https://github.com/apache/openjpa/pull/79#discussion_r611061131
##########
File path: pom.xml
##########
@@ -1102,6 +1102,58 @@
</properties>
</profile>
+ <!-- Profile for testing with HerdDB -->
+ <profile>
+ <id>test-herddb-docker</id>
+ <activation>
+ <property>
+ <name>test-herddb-docker</name>
+ </property>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>org.herddb</groupId>
+ <artifactId>herddb-jdbc</artifactId>
+ <version>0.22.0</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+ <properties>
+ <connection.driver.name>herddb.jdbc.Driver</connection.driver.name>
Review comment:
what about a plain java test since herddb is java? docker is mainly an additional test to enable to test which looks not needed here, wdyt?
--
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] [openjpa] eolivelli commented on pull request #79: HerdDB profile for integration tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #79:
URL: https://github.com/apache/openjpa/pull/79#issuecomment-817279766
I added a few references to the docs about the ways to run HerdDB
probably many failures are due to:
- missing using of "delimiters"
- unsupported syntaxes in HerdDB (all in all it is a new dialect....)
This is a minimal test case project we have in HerdDB repository to ensure that OpenJPA is working with the basic features
https://github.com/diennea/herddb/tree/master/herddb-thirdparty/openjpa-test
--
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] [openjpa] eolivelli commented on a change in pull request #79: HerdDB profile for integration tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #79:
URL: https://github.com/apache/openjpa/pull/79#discussion_r611066345
##########
File path: pom.xml
##########
@@ -1102,6 +1102,58 @@
</properties>
</profile>
+ <!-- Profile for testing with HerdDB -->
+ <profile>
+ <id>test-herddb-docker</id>
+ <activation>
+ <property>
+ <name>test-herddb-docker</name>
+ </property>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>org.herddb</groupId>
+ <artifactId>herddb-jdbc</artifactId>
+ <version>0.22.0</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+ <properties>
+ <connection.driver.name>herddb.jdbc.Driver</connection.driver.name>
Review comment:
the main problem is about adding more jars in the classpath.
I am fine with using in-memory db as well, tests will be probably faster?
we should have both.
I would start with docker in order to not need to spend time in debugging potential problems about re-initialization of the server
--
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] [openjpa] eolivelli commented on pull request #79: HerdDB profile for integration tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #79:
URL: https://github.com/apache/openjpa/pull/79#issuecomment-817279338
@rmannibucau @struberg I am able to run the tests using HerdDB is pure "embedded" mode.
the problem is that there are many tests that are failing.
When I developed the fixes for OpenJPA I did not run all of the tests, but only a basic subset.
I have updated the patch, with `-Dtest-herddb ` you run the tests using HerdDB
`mvn clean install -Dtest-herddb`
--
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] [openjpa] rmannibucau commented on pull request #79: HerdDB profile for integration tests
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #79:
URL: https://github.com/apache/openjpa/pull/79#issuecomment-817292009
We can use assume to skip unsupported tests I guess for now and flag them explicitly as "todo" maybe? (For next release to not block it)
--
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] [openjpa] rmannibucau commented on a change in pull request #79: HerdDB profile for integration tests
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #79:
URL: https://github.com/apache/openjpa/pull/79#discussion_r611069648
##########
File path: pom.xml
##########
@@ -1102,6 +1102,58 @@
</properties>
</profile>
+ <!-- Profile for testing with HerdDB -->
+ <profile>
+ <id>test-herddb-docker</id>
+ <activation>
+ <property>
+ <name>test-herddb-docker</name>
+ </property>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>org.herddb</groupId>
+ <artifactId>herddb-jdbc</artifactId>
+ <version>0.22.0</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+ <properties>
+ <connection.driver.name>herddb.jdbc.Driver</connection.driver.name>
Review comment:
I would dependency:unpack the zip/stack and just launch it with a dedicated classloader (no conflict wth openjpa)
Docker is fine but don't expect it to be ran often except on the CI from time to time - this is my main point, only Mark runs docker profiles AFAIK.
--
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] [openjpa] struberg commented on a change in pull request #79: HerdDB profile for integration tests
Posted by GitBox <gi...@apache.org>.
struberg commented on a change in pull request #79:
URL: https://github.com/apache/openjpa/pull/79#discussion_r616123557
##########
File path: openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/TestUnwrap.java
##########
@@ -30,12 +30,22 @@
import org.apache.openjpa.jdbc.conf.JDBCConfiguration;
import org.apache.openjpa.jdbc.sql.DerbyDictionary;
+import org.apache.openjpa.jdbc.sql.HerdDBDictionary;
import org.apache.openjpa.kernel.QueryLanguages;
import org.apache.openjpa.lib.jdbc.DelegatingConnection;
+import org.apache.openjpa.persistence.detachment.model.NoDetachedStateEntityFieldAccess;
+import org.apache.openjpa.persistence.detachment.model.NoDetachedStateEntityPropertyAccess;
import org.apache.openjpa.persistence.test.SingleEMFTestCase;
+import static org.junit.Assume.assumeFalse;
+
public class TestUnwrap extends SingleEMFTestCase {
+ @Override
+ protected void setUp(Object... props) {
+ assumeFalse(this.getDBDictionary() instanceof HerdDBDictionary);
Review comment:
do you just want to skip this test for HerdDB? In which case you can simply call `setUnsupportedDatabases(HerdDBDictionary.class)`
--
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] [openjpa] eolivelli commented on pull request #79: HerdDB profile for integration tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #79:
URL: https://github.com/apache/openjpa/pull/79#issuecomment-821803174
@rmannibucau @struberg I am adding assumptions in order to skip test that fail to HerdDB.
some tests fail due to assertions that do not keep into consideration that the dictionary is applying delimiters, some tests are due to some other syntax error (probably something not supported by Calcite) and some other errors fail due to some DB error that needs more investigation.
--
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] [openjpa] eolivelli commented on a change in pull request #79: HerdDB profile for integration tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #79:
URL: https://github.com/apache/openjpa/pull/79#discussion_r616132439
##########
File path: openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/TestUnwrap.java
##########
@@ -30,12 +30,22 @@
import org.apache.openjpa.jdbc.conf.JDBCConfiguration;
import org.apache.openjpa.jdbc.sql.DerbyDictionary;
+import org.apache.openjpa.jdbc.sql.HerdDBDictionary;
import org.apache.openjpa.kernel.QueryLanguages;
import org.apache.openjpa.lib.jdbc.DelegatingConnection;
+import org.apache.openjpa.persistence.detachment.model.NoDetachedStateEntityFieldAccess;
+import org.apache.openjpa.persistence.detachment.model.NoDetachedStateEntityPropertyAccess;
import org.apache.openjpa.persistence.test.SingleEMFTestCase;
+import static org.junit.Assume.assumeFalse;
+
public class TestUnwrap extends SingleEMFTestCase {
+ @Override
+ protected void setUp(Object... props) {
+ assumeFalse(this.getDBDictionary() instanceof HerdDBDictionary);
Review comment:
@struberg thanks for the hint.
I wasn't aware of setUnsupportedDatabases
I will rework this patch
--
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