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

Test: hadoopfs tests remove the use of @Value.Immutable #8572

Closed
wants to merge 6 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions clients/hadoopfs/pom.xml
Original file line number Diff line number Diff line change
@@ -391,12 +391,6 @@ To export to S3:
<version>5.15.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.immutables</groupId>
<artifactId>value</artifactId>
<version>2.9.3</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
25 changes: 4 additions & 21 deletions clients/hadoopfs/src/test/java/io/lakefs/FSTestBase.java
Original file line number Diff line number Diff line change
@@ -1,15 +1,7 @@
package io.lakefs;

import com.amazonaws.ClientConfiguration;
import com.amazonaws.auth.AWSCredentials;
import com.amazonaws.auth.BasicAWSCredentials;
import com.amazonaws.services.s3.AmazonS3;
import com.amazonaws.services.s3.AmazonS3Client;
import com.amazonaws.services.s3.S3ClientOptions;
import com.amazonaws.services.s3.model.*;
import com.aventrix.jnanoid.jnanoid.NanoIdUtils;
import com.google.common.base.Optional;
import com.google.common.collect.ImmutableMap;
import com.google.gson.FieldNamingPolicy;
import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
@@ -20,27 +12,19 @@
import io.lakefs.utils.ObjectLocation;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileAlreadyExistsException;
import org.apache.hadoop.fs.FileStatus;
import org.apache.hadoop.fs.LocatedFileStatus;
import org.apache.hadoop.fs.Path;
import org.apache.http.HttpStatus;

import org.immutables.value.Value;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TestName;

import org.mockserver.client.MockServerClient;
import org.mockserver.junit.MockServerRule;
import org.mockserver.matchers.MatchType;
import org.mockserver.matchers.TimeToLive;
import org.mockserver.matchers.Times;
import org.mockserver.model.Cookie;
import org.mockserver.model.HttpRequest;
import org.mockserver.model.HttpResponse;
import org.mockserver.model.Parameter;

import static org.apache.commons.lang3.StringUtils.removeStart;

@@ -51,7 +35,6 @@
import java.net.URI;
import java.net.URISyntaxException;
import java.util.Arrays;
import java.util.List;

/**
* Base for all LakeFSFilesystem tests. Helps set common components up but
@@ -85,10 +68,10 @@ public abstract class FSTestBase {
.setFieldNamingPolicy(FieldNamingPolicy.LOWER_CASE_WITH_UNDERSCORES)
.create();

@Value.Immutable static public interface Pagination {
@Value.Parameter Optional<Integer> amount();
@Value.Parameter Optional<String> after();
@Value.Parameter Optional<String> prefix();
static public interface Pagination {
Optional<Integer> amount();
Optional<String> after();
Optional<String> prefix();
}

@Rule
56 changes: 56 additions & 0 deletions clients/hadoopfs/src/test/java/io/lakefs/ImmutablePagination.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package io.lakefs;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand: this used to be auto-generated; is this now manually written?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


import com.google.common.base.Optional;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the standard java.util.Optional instead? (Maybe not if code is generated with a different Optional...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I thought the code prefered Google's package so we left it.

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 code is used in a single location - and already used 'if' to check the optional value. So I've updarted to check for null.


public final class ImmutablePagination implements FSTestBase.Pagination {
private final Optional<Integer> amount;
private final Optional<String> after;
private final Optional<String> prefix;

private ImmutablePagination(Optional<Integer> amount, Optional<String> after, Optional<String> prefix) {
this.amount = amount;
this.after = after;
this.prefix = prefix;
}

public Optional<Integer> amount() {
return this.amount;
}

public Optional<String> after() {
return this.after;
}

public Optional<String> prefix() {
return this.prefix;
}

public static Builder builder() {
return new Builder();
}

public static class Builder {
private Optional<Integer> amount = Optional.absent();
private Optional<String> after = Optional.absent();
private Optional<String> prefix = Optional.absent();

public Builder amount(Integer amount) {
this.amount = Optional.fromNullable(amount);
return this;
}

public Builder after(String after) {
this.after = Optional.fromNullable(after);
return this;
}

public Builder prefix(String prefix) {
this.prefix = Optional.fromNullable(prefix);
return this;
}

public ImmutablePagination build() {
return new ImmutablePagination(amount, after, prefix);
}
}
}
Original file line number Diff line number Diff line change
@@ -5,40 +5,22 @@

import io.lakefs.clients.sdk.*;
import io.lakefs.clients.sdk.model.*;
import io.lakefs.clients.sdk.model.ObjectStats.PathTypeEnum;
import io.lakefs.utils.ObjectLocation;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import org.apache.commons.io.IOUtils;
import org.apache.hadoop.fs.FileStatus;
import org.apache.hadoop.fs.LocatedFileStatus;
import org.apache.hadoop.fs.Path;
import org.apache.http.HttpStatus;
import org.junit.Assert;
import org.junit.Test;

import org.hamcrest.core.StringContains;

import org.mockserver.client.MockServerClient;
import org.mockserver.matchers.TimeToLive;
import org.mockserver.matchers.Times;
import org.mockserver.model.Cookie;
import org.mockserver.model.HttpRequest;
import org.mockserver.model.HttpResponse;
import org.mockserver.model.Parameter;

import static org.mockserver.model.HttpResponse.response;
import static org.mockserver.model.JsonBody.json;

import java.io.*;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

public class LakeFSFileSystemServerTest extends FSTestBase {