Skip to content
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

TCK has outdated expectation for exception type #593

Merged

Conversation

njr-11
Copy link
Contributor

@njr-11 njr-11 commented Mar 26, 2024

While investigating exceptions that are trapped in the TCK, I found a bunch of places where the TCK is trapping for MappingException that should have been updated when the specification was switched to UnsupportedOperationException. Fortunately, these are all cases where the requirement for the exception is at run time only because it requires finding out information from the database. Otherwise we would have a problem testing the behavior at all in these cases.

@njr-11 njr-11 added bug Something isn't working test Something test-related labels Mar 26, 2024
@njr-11 njr-11 added this to the Jakarta Data 1.0 milestone Mar 26, 2024
@njr-11 njr-11 force-pushed the tck-traps-for-outdated-exception-type branch from d2b414f to c10d037 Compare March 26, 2024 18:12
Copy link
Contributor Author

@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.

When running the TCK, I found that MappingException has hiding a few bugs in the TCK. See the following comments for explanations of what those errors are.

@@ -41,7 +41,7 @@ public interface PositiveIntegers extends BasicRepository<NaturalNumber, Long> {

boolean existsByIdGreaterThan(Long number);

CursoredPage<NaturalNumber> findByFloorOfSquareRootNotAndIdLessThanOrderByBitsRequiredDesc(long excludeSqrt,
CursoredPage<NaturalNumber> findByFloorOfSquareRootNotAndIdLessThanOrderByNumBitsRequiredDesc(long excludeSqrt,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Query by Method Name was using incorrect field name that did not match the entity.

Comment on lines +706 to +711
// 32 requires 6 bits
// 25, 26, 27, 28, 29, 30, 31 requires 5 bits
// 8, 9, 10, 11, 12, 13, 14, 15 requires 4 bits
// 4, 5, 6, 7, 8 requires 3 bits
// 2, 3 requires 2 bits
// 1 requires 1 bit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case was trying to assert ordering by square root descending but it actually asks for sorting by number of bits required descending.

// is not capable of cursor-based pagination.
return;
}

assertEquals(Arrays.toString(new Long[] { 10L, 9L, // 4 bits required, square root rounds down to 3
7L, 6L, 5L, 4L, // 3 bits required, square root rounds down to 2
3L, 2L // 2 bits required, square root rounds down to 1
3L // 2 bits required, square root rounds down to 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case asks for pages of size 7, but mistakenly tries to assert that this page will have 8 elements.

Copy link
Contributor

@KyleAure KyleAure left a comment

Choose a reason for hiding this comment

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

LGTM

@otaviojava otaviojava merged commit 84ac92a into jakartaee:main Mar 27, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working test Something test-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants