-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Migrate to Java 16 records (part 2) #82914
Conversation
b7abdf8
to
ccf09a1
Compare
Pinging @elastic/es-core-infra (Team:Core/Infra) |
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.
LGTM! Thanks Artem.
I left two questions about some small test changes.
ShardSegments shardSegmentsOne = Mockito.mock(ShardSegments.class); | ||
ShardSegments[] shardSegmentsArray = new ShardSegments[] { shardSegmentsOne }; | ||
IndexShardSegments indexShardSegments = new IndexShardSegments(ShardId.fromString("[idx][123]"), shardSegmentsArray); |
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.
Just curious, what was the reason for this test change?
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 used to be a mock via Mockito.mock(IndexShardSegments.class)
, but with the change IndexShardSegments
is now a record which is final
. Mockito can't mock final classes without additional Java agent instrumentation. So we have to use a "real" data class here.
final Build mockBuild = mock(Build.class); | ||
when(mockBuild.flavor()).thenReturn(Build.Flavor.DEFAULT); | ||
when(mockBuild.type()).thenReturn(Build.Type.DOCKER); | ||
final Build mockBuild = new Build(Build.Flavor.DEFAULT, Build.Type.DOCKER, "", "", false, ""); |
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 question about this test change.
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.
Same purpose here. We can't mock records and Mockito usually advice against mocking data classes.
Thanks Nikola! |
💔 Backport failed
You can use sqren/backport to manually backport by running |
Backports elastic#82914 to 8.0
Try to represent immutable data with Java records introduced in JEP 395
Convert only existing immutable classes, no "POJO with setters to a record" refactorings.