Skip to content

Move query-by-name methods out to "dedicated" repositories/tests#1484

Open
marko-bekhta wants to merge 1 commit into
jakartaee:mainfrom
marko-bekhta:fix/move-query-by-name-to-different-repositry-test-class
Open

Move query-by-name methods out to "dedicated" repositories/tests#1484
marko-bekhta wants to merge 1 commit into
jakartaee:mainfrom
marko-bekhta:fix/move-query-by-name-to-different-repositry-test-class

Conversation

@marko-bekhta
Copy link
Copy Markdown
Member

So that it is possible to run the tests without needing to generate the query-by-method-name repository methods.

Copy link
Copy Markdown
Member

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although some of the TCK classes could benefit from refactoring, I don't agree with this approach that tries to create a separation between the Query by Method Name test cases and all the others. Given that Query by Method Name was the predominant pattern in Data 1.0 (@Find didn't have Constraints yet), most of the tests of useful Data 1.0 function happened to use the Query by Method Name pattern. So a refactoring based on what is using Query by Method Name versus what is not is arbitrary and a very poor way to split up the TCK -- one that risks losing a lot of coverage for core scenarios completely unrelated to Query by Method Name if Query by Method Name is ever deprecated and dropped from the spec. Also, any refactoring that is done ought to be more meaningful and done in smaller, more reviewable chunks.

@gavinking
Copy link
Copy Markdown
Member

gavinking commented Jun 4, 2026

@njr-11 I asked Marko to do this for three reasons:

  1. It's simply good practice to keep tests for independent functionality independent. It's not a good practice to mix together tests for unrelated functionality into one big blob. In the lifetime of the Hibernate project more than one attempt was made to have a "shared model" reused in multiple tests, and that always led to a complete mess where, bit by bit, the "shared model" turned into a dumping ground of stuff which wasn't shared at all, but rather specific to one or two tests.
  2. Our implementation of Jakarta Data is split across two modules. QBMN is implemented as part of WildFly, and the rest is implemented in Hibernate. We would like to run the TCK on Hibernate CI. The current setup prevents us from doing that.
  3. We're going to have to do this split eventually anyway, since in Jakarta Data 2, QBMN will no longer be required.

Given that Query by Method Name was the predominant pattern in Data 1.0

I don't agree with this. Nobody using Jakarta Data with plain Hibernate or on Quarkus even has QBMN as an option. We supply it in WildFly for EE spec compliance.

if Query by Method Name is ever deprecated and dropped from the spec

Query by Method Name is already explicitly deprecated.

@gavinking
Copy link
Copy Markdown
Member

Also, any refactoring that is done ought to be more meaningful and done in smaller, more reviewable chunks.

P.S. I you're concerned that any test assertions or test coverage might be lost, I can get GPT-5.5 to do a careful review across the two branches and look for any discrepancies. I have complete confidence it will find any that exist. This is not the sort of thing humans should be wasting time on.

@njr-11
Copy link
Copy Markdown
Member

njr-11 commented Jun 4, 2026

Given that Query by Method Name was the predominant pattern in Data 1.0

I don't agree with this. Nobody using Jakarta Data with plain Hibernate or on Quarkus even has QBMN as an option. We supply it in WildFly for EE spec compliance.

That is taken out of context. What I mean by this is that if a Data 1.0 test case intended to cover some core functionality like pagination, and it wants to perform a basic query such as less than, the simplest way to do that in Data 1.0 is to use Query by Method Name. We didn't have the LessThan Constraint type, so it was perfectly reasonable for a Data 1.0 test of pagination to use findByXLessThan, and so forth. Query by Method Name is the predominant pattern used by the Data 1.0 TCK for good reason.

As we are all aware, Query by Method Name was not moved to its own separate module until the very end of Data 1.0, well after the 1.0 TCK had already been written. At the time the only alternative would have been to use @Query everywhere, but there is not much value in testing over and over again that a Data provider can hand queries on to JPA. The right way to refactor these would be to take the following steps:

  1. Write new tests explicitly covering the all of the Query by Method Name patterns and include the needed logic so that you can skip this part in your CI.
  2. Replace usage of Query by Method name within the existing tests by switching to @Find and @Delete with Constraints.
  3. In a few cases (2) won't be possible, which could lead to some new requirements for Data vNext.
  4. Create new test classes for different categories of tests (Limit, Offset Pagination, Cursor Pagination, ...) that can be split out from EntityTest.

Hopefully most of the tests that have already been added in Data 1.1 will be more acceptable because. They have been aiming to follow better practices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants