Test: hadoopfs tests remove the use of @Value.Immutable #8572
Conversation
- Added ImmutablePagination class to handle pagination parameters in tests. - Updated test setup to use ImmutablePagination class for pagination parameters. - This change removes the use of @Value.Immutable in the test and works with latest Java compilers.
nopcoder
added
area/testing
E2E Test Results - DynamoDB Local - Local Block Adapter |
E2E Test Results - Quickstart |
There was a problem hiding this comment.
Thanks!
But frankly I am not sure why this would be required. E.g. the docs (and even the changelog) say nothing about not working with a modern JVM. Ideally we would link to an issue that says immutable Values don't work with a modern Java. Maybe just bump to 2.10.1? Or are we using it incorrectly???
I am not a fan of writing generated code by hand. For instance the code generated by immutable.Value gives actual value types; the type here does not. "Value type" means, for instance, the existence of working equals and hashCode methods. You also get any number of great routines; see here.
| @@ -0,0 +1,56 @@ | |||
| package io.lakefs; | |||
There was a problem hiding this comment.
I don't understand: this used to be auto-generated; is this now manually written?
There was a problem hiding this comment.
Used the common builder pattern to implement the Pagination class.
This class is used by the test code to pass pagination argumens (some optional).
If Java add optional field initialization I'm sure we didn't had this class in the first place. We do not use the generated code in non-test.
| @@ -0,0 +1,56 @@ | |||
| package io.lakefs; | |||
|
|
|||
| import com.google.common.base.Optional; | |||
There was a problem hiding this comment.
Can we use the standard java.util.Optional instead? (Maybe not if code is generated with a different Optional...)
There was a problem hiding this comment.
Sure, I thought the code prefered Google's package so we left it.
There was a problem hiding this comment.
This code is used in a single location - and already used 'if' to check the optional value. So I've updarted to check for null.
|
Updated the description with the open issue from the immutables package and simplify the code. |
Add Pagination implementation to replace the current usage of @Value.Immutable in the test code.
The two main drives to this change:
The current test code define interface for pagination information an using code generation based on the interface, uses a builder to pass the pagination information to the test code.
Think this is a bit over engineerig and can be simplify by just using the same class with generated builder (also not required).
The use of Optional class was also removed as we just check for null.
If you think it is too much and we should drive for no
nullcheck code and check of absent I can revert that change. But my drive to make the code simple.Note that this change doesn't solve the issue of Java dependency, as Hadoop requires it. but it reduce the dependency and simplify the build process as not code generation is required for running the tests.
PS removed dup entry in the packages list