-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Informix tests fixes #8874
base: main
Are you sure you want to change the base?
Informix tests fixes #8874
Conversation
Thanks for your pull request! This pull request does not follow the contribution rules. Could you have a look? ❌ All commit messages should start with a JIRA issue key matching pattern › This message was automatically generated. |
I'm not sure we want to spread special cases across the main test suite for a community-supported dialect. |
hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/FunctionTests.java
Outdated
Show resolved
Hide resolved
49b5951
to
9a915dc
Compare
9a915dc
to
b5fa9b2
Compare
I believe it is legitimate for tests to also consider community dialects. Especially if the database is also available in Docker and it is documented in the README. What do you think about that @beikov ?" |
I suggest including community dialect tests in CI build, but configured to allow the build to continue even if these tests fail. This would enable to monitor the evolution of dialect support and detect regressions without jeopardizing the overall CI process. |
@VladoKuruc as I already said, your PR looks fine but I'm reluctant to special-case community dialects in the core test suite. Also, testing community-supported dialects is up to the user / vendor that maintains them and using CI would consume resources. However, I can bring this up with the team and see what they think of it. Note: you can just skip the failing tests locally while they are not yet implemented / supported by the dialect you're testing |
hibernate-core/src/test/java/org/hibernate/orm/test/type/UUIDTypeConverterTest.java
Outdated
Show resolved
Hide resolved
...rnate-core/src/test/java/org/hibernate/orm/test/id/uuid/sqlrep/sqlbinary/UUIDBinaryTest.java
Outdated
Show resolved
Hide resolved
hibernate-core/src/test/java/org/hibernate/orm/test/id/array/PrimitiveByteArrayIdTest.java
Outdated
Show resolved
Hide resolved
@@ -58,6 +59,7 @@ public class Forest { | |||
|
|||
@OptimisticLock(excluded=true) | |||
@JdbcTypeCode( Types.LONGVARCHAR ) | |||
@Column(length = 10000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I believe that using the default length for LONGVARCHAR
which should be Length.LONG
is on purpose. Why do you have to change the size here? If varchar
has a max capacity, it should be handled by implementing Dialect#getMaxVarcharLength()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that the maximum length for the LONGVARCHAR
type is set in the getMaxVarcharLength()
method, which is correctly set to 32739 for Informix. The problem is that Informix also has a row length limit. The Forest entity is only used in the test org.hibernate.orm.test.annotations.entity.BasicHibernateAnnotationsTest
, where I don't think the column length is critical.
create table Forest (
id integer not null,
length int8 not null,
longDescription lvarchar(32600),
bigText varchar(255),
name varchar(255),
smallText varchar(255),
country blob,
primary key (id)
)" via JDBC [The operation causes a rowsize to exceed the allowable limit (32767).]
Of course, I could ignore these tests for InformixDialect, but I think more coverage is beneficial.
hibernate-core/src/test/java/org/hibernate/orm/test/jpa/criteria/QueryBuilderTest.java
Outdated
Show resolved
Hide resolved
hibernate-core/src/test/java/org/hibernate/orm/test/query/CteTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than what I commented, this looks ok to me.
b5fa9b2
to
81432c5
Compare
81432c5
to
449fa85
Compare
Fixes for some of the Informix tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.