-
Notifications
You must be signed in to change notification settings - Fork 34
Include first test to Jakarta Data Core [Part 1] #1318
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
base: main
Are you sure you want to change the base?
Conversation
…f intermediate variables
…eListSupplier implementation
…s, include AssertJ dependency in build file
…a-driven test assertions
…tities test for enhanced metadata and parameterized execution
… with corresponding tests in JakartaQueryTests
…ts with dynamic parameter handling - Introduced several query methods in `FruitRepository` for filtering by name, quantity, and ranges. - Updated `JakartaQueryTests` to adjust database keyword support checks and added a nested class for dynamic parameters.
…Tests setup - Introduced `deleteAll` method in `FruitRepository` for bulk deletion. - Updated `JakartaQueryTests` to use `fruitRepository.deleteAll` in test setup for data cleanup.
…rtaQueryTests with additional comparisons
…ltering and update JakartaQueryTests assertions
… JakartaQueryTests - Updated `FruitRepository` methods to use accurate parameter annotations and improve query logic. - Added parameterized tests for AND/OR queries in `JakartaQueryTests` with comprehensive assertions.
njr-11
left a comment
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.
There is too much combined in a single PR. Parts of this are unrelated and should be reviewed separately.
For example, alterations to existing TCK tests, which need to be better understood on their own,
- Removed an unused or incorrect WHERE clause from a query in
NaturalNumbers.javato clean up the code.
The WHERE clause is not unused because you had to alter test assertions after removing it. I don't know what would be incorrect about it. If you are seeing an error, we need to explore why. Are there additional limitations of NoSQL databases (other than Key-Value and Column, which are already excluded) causing problems here? If so, we should add and document an appropriate exclusion.
I haven't looked at the other parts other than seeing that there is a Faker class. Hopefully that does not produce random data. We should avoid randomness so that TCK failures reproduce every time the tests run. Otherwise someone can just keep retrying until they are lucky and then claim compliance.
- Enhanced `numberPage` query with additional filtering condition. - Updated `EntityTests` assertions to reflect refined query results and improve consistency.
|
@njr-11 I've removed the natural numbers, and I will move it to another PR.
This part of the code uses data-driven testing. Faker is only used to generate input parameters; once those parameters are provided, test execution and assertions are fully deterministic. If this is a concern, I’m happy to change it. Still, when the focus is on validating behavior across data variations, a data-driven approach is, in my view, the most appropriate and practical choice. |
…itSupplier with static data - Removed `AbstractListSupplier` class and `DataFaker` dependency. - Renamed `FruitListSupplier` to `FruitSupplier` and replaced dynamic fruit generation with static list initialization.
… FruitSupplier to use static fruit data
…pplier, remove unused imports
…and add FruitSummary record
…and add a new test for projection
…and add a new test for projection
…treamline deployment configuration
Yes, @KyleAure did an amazing job! And I never said anything different! My point is: we need to keep updating based on the context and challenges that we have now! |
|
@njr-11, how about now? We got 7 files. |
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Outdated
Show resolved
Hide resolved
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/FruitPopulator.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Standalone | ||
| @AnyEntity | ||
| @ReadOnlyTest |
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.
Similar to previous review comment, you only need to add the @ReadOnlyTest annotation if you are using a repository from the read.only package. See: https://github.com/jakartaee/data/blob/main/tck/src/main/java/ee/jakarta/tck/data/framework/arquillian/extensions/TCKArchiveProcessor.java#L67-L71
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've removed this annotation once, I will include operations that will change the data.
|
FWIW: I agree using modernized testing strategies like parameterized tests is valid, but we have to make sure we have a valid use case to do it. Before your recent changes I was going to comment about the fact that in each test case you attempted to persist the same list of entities, which would have caused EntityExistsException(s) had it been run against a database. Also remember that many Jakarta users look at the TCK tests to see how to use entities/repositories/queries so the more test abstraction we add the less these tests resemble what user might actually write in their application and therefore be poor examples of valid use cases. |
njr-11
left a comment
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.
Thanks - I completed reviewing the tests. I found a lot of issues with the allowances, usually stating the wrong reason and sometimes applying to database types that are inconsistent with what we already have in the Data TCK.
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Show resolved
Hide resolved
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Show resolved
Hide resolved
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Show resolved
Hide resolved
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Show resolved
Hide resolved
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Show resolved
Hide resolved
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Outdated
Show resolved
Hide resolved
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Show resolved
Hide resolved
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Outdated
Show resolved
Hide resolved
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Outdated
Show resolved
Hide resolved
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/Transmission.java
Outdated
Show resolved
Hide resolved
…pabilities for query operations
|
@njr-11 I've updated all the comments |
|
@KyleAure, I've changed what you said. Could you check it as well? |
| import java.util.logging.Logger; | ||
|
|
||
| @Standalone | ||
| @AnyEntity |
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.
| @AnyEntity | |
| @ReadOnlyTest | |
| @AnyEntity |
I think something got mis-interpreted with my previous suggestions. Having the @ReadOnlyTest annotation was a good thing, but the issue was that you didn't actually put the classes in the read only package.
Add back in the annotation, but move the entity and repository to the read only package.
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.
But @KyleAure, the next commit will write, those are only the first 10 of 50 tests.
IMHO: it does not make sense to move to ReadOnly, and the next PR move it again.
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.
That was not clear to me based off of how you were using the repository in this PR. (I cannot predict the future :D)
However, the more performant thing to do would be to have one repository (Fruits) that only does read operation testing and keep it read only, then have another repository (Vegetables?) that does read/write operations.
That way on a per-test basis you can ensure that whatever you wrote to the read/write repository is undone before moving onto the next test?
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 cannot predict the future :D
My bad :)
Can I use this Fruit entity for both writing and reading?
Or should I have one entity for reading operations and one for writing operations?
That is not clear to me.
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.
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Show resolved
Hide resolved
|
@njr-11 also, don't forget to review it as well. |
njr-11
left a comment
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.
It looks like my prior review comments were blasted away, so I tried to go through everything again and post comments.
tck/src/main/java/ee/jakarta/tck/data/standalone/entity/JakartaQueryTests.java
Outdated
Show resolved
Hide resolved
| .containsAll(fruits); | ||
| } catch (UnsupportedOperationException exp) { | ||
| if (type.isKeywordSupportAtOrBelow(DatabaseType.COLUMN)) { | ||
| // Column and Key-Value databases might not be capable of find all elements. |
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 don't see any language in the spec that makes this allowance
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.
If in both cases we can find by ID, and we are running a query without ID, then we won't be able to execute this query.
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.
The point is that the specification doesn't allow for the exemption. You need spec language to cover that. I just added that for you in #1319 , after which this one should now be okay.
| .containsAll(expected); | ||
| } catch (UnsupportedOperationException exp) { | ||
| if (type.isKeywordSupportAtOrBelow(DatabaseType.COLUMN)) { | ||
| // Column and Key-Value databases might not be capable of find all elements. |
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.
Again, I haven't been able to find any language in the spec allowing this to be ignored for key-value and column
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.
@njr-11, do you prefer to update the query and put in (ids) instead?
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.
See the discussion of my previous comment. I proposed adding spec language in #1319 that would make the allowance here valid.
| .allMatch(fruit -> !fruit.getName().equals(sample.getName())); | ||
| } catch (UnsupportedOperationException exp) { | ||
| if (type.isKeywordSupportAtOrBelow(DatabaseType.COLUMN)) { | ||
| // Column and Key-Value databases might not be capable querying by attribute that is not a key. |
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.
Comparing an attribute that is not the key is making a lot of these very basic tests not apply to Column and Key-Value. I recommend we switch the repository method so that its JCQL query compares the key value instead. Then we won't need to skip so many tests.
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 can do it, but from the user perspective, why not use the method that we have built in, such as findById?
I am fine to update it, but I would search non-ID attributes.
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.
Please, let me know what you think, thus, I can update as you wish.
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 can do it, but from the user perspective, why not use the method that we have built in, such as
findById?
Because this is a TCK test of query language, not a user example that limits itself to the easiest way to use the API. I'm pretty sure we already have TCK tests for findById versus the valid query language WHERE id = :id which is probably not covered anywhere.
…aQueryTests.java Co-authored-by: Nathan Rauh <[email protected]>
This pull request introduces a set of new entity classes, repositories, and supporting utilities for standalone data-driven tests in the TCK. It also adds AssertJ as a test dependency and provides a custom annotation for parameterized assertions. The changes focus on enabling richer test scenarios with entities like
FruitandVehicle, their suppliers, and repositories, as well as improving test metadata and assertion capabilities.Entity and Repository Additions
Fruit,Vehicle, and supporting enumTransmission, each with fields, constructors, and standard methods for equality and string representation. [1] [2] [3]FruitRepositoryandVehicleRepository, providing query methods for CRUD and filtering operations on the respective entities. [1] [2]VehicleSummaryfor summarizing vehicle data in queries.Data Suppliers for Parameterized Tests
FruitSupplierandVehicleSupplierto provide lists of sample entities for parameterized tests, backed by static lists and utility methods for entity creation. [1] [2]AbstractSupplierimplementing bothSupplier<List<T>>and JUnit'sArgumentsProviderfor seamless integration with parameterized testing.Test Framework and Metadata Enhancements
ParametizedAssertionfor tracking test metadata, such as GitHub issue/PR references and assertion strategies, to improve test documentation and traceability.tck/pom.xmlto include AssertJ as a dependency for fluent assertions in tests, and defined its version in the properties section. [1] [2]