Refactor TestHttpCredentialsProvider #159
Conversation
| public static class Filter | ||
| implements BuilderFilter | ||
| { | ||
| private static String httpEndpointUri; |
There was a problem hiding this comment.
This should be a local variable
|
|
||
| import jakarta.servlet.Servlet; | ||
|
|
||
| public interface TestHttpCredentialsServlet |
There was a problem hiding this comment.
This should probably be named closer to its purpose. Maybe TestHttpCredentialsCounts or something like that.
There was a problem hiding this comment.
This could perhaps also be moved to the testing package
| import jakarta.servlet.Servlet; | ||
|
|
||
| public interface TestHttpCredentialsServlet | ||
| extends Servlet |
|
|
||
| TestHttpCredentialsServlet httpCredentialsServlet; | ||
| try { | ||
| httpCredentialsServlet = new TestHttpCredentialsServletImpl(expectedHeaders); |
There was a problem hiding this comment.
| httpCredentialsServlet = new TestHttpCredentialsServletImpl(expectedHeaders); | |
| TestHttpCredentialsServletImpl testHttpCredentialsServlet = new TestHttpCredentialsServletImpl(expectedHeaders); | |
| httpCredentialsServlet = testHttpCredentialsServlet; | |
| httpCredentialsServer = createTestingHttpCredentialsServer(testHttpCredentialsServlet); |
Then you can remove the extends Servlet
|
|
||
| import static jakarta.ws.rs.core.MediaType.APPLICATION_JSON; | ||
|
|
||
| public class TestHttpCredentialsServletImpl |
There was a problem hiding this comment.
I don't think we need a generic interface for this, it's a Testing HTTP server, we could remove the interface and make everything use this class
| private static String httpEndpointUri; | ||
|
|
||
| @Override | ||
| public TestingTrinoAwsProxyServer.Builder filter(TestingTrinoAwsProxyServer.Builder builder) |
There was a problem hiding this comment.
This filter, unlike the TestingHttpCredentialsServlet, would be of limited use to anything except for a single test class (where it was moved from), I think that it should be moved back under TestHttpCredentialsProvider
pranavr12
force-pushed
the
refactor-http-creds-tests
branch
from
6e52c80
to
8796293
Compare
...oxy/src/test/java/io/trino/aws/proxy/server/credentials/http/TestHttpCredentialsServlet.java
Outdated
Show resolved
Hide resolved
...oxy/src/test/java/io/trino/aws/proxy/server/credentials/http/TestHttpCredentialsServlet.java
Outdated
Show resolved
Hide resolved
| @@ -186,74 +180,12 @@ private void testNoCredentialsRetrieved(String emulatedAccessKey, Optional<Strin | |||
| assertThat(httpCredentialsServlet.getRequestCount()).isEqualTo(1); | |||
| } | |||
|
|
|||
| private static TestingHttpServer createTestingHttpCredentialsServer(HttpCredentialsServlet servlet) | |||
| protected static TestingHttpServer createTestingHttpCredentialsServer(Servlet servlet) | |||
There was a problem hiding this comment.
this could be public and moved to TestingUtils or some other class since its not specific to the credentials server. we have another function called createTestingHttpErrorResponseServer which does the same thing and could be replaced by this
pranavr12
force-pushed
the
refactor-http-creds-tests
branch
2 times, most recently
from
e4433ca
to
17edac4
Compare
trino-aws-proxy/src/test/java/io/trino/aws/proxy/server/testing/TestingUtil.java
Outdated
Show resolved
Hide resolved
pranavr12
force-pushed
the
refactor-http-creds-tests
branch
from
17edac4
to
291c6da
Compare
pranavr12
force-pushed
the
refactor-http-creds-tests
branch
from
291c6da
to
4ed6436
Compare
| @@ -148,4 +154,13 @@ public static String sha256(String content) | |||
| { | |||
| return Hashing.sha256().newHasher().putString(content, StandardCharsets.UTF_8).hash().toString(); | |||
| } | |||
|
|
|||
| public static TestingHttpServer createTestingHttpServer(Servlet servlet) | |||
There was a problem hiding this comment.
Could we reuse this in TestProxiedErrorResponses please?
The refactor allows reusing of classes for custom implementations of the HTTP Credentials Provider