suneet-s commented on pull request #10098:
URL: https://github.com/apache/druid/pull/10098#issuecomment-654385127
> It looks like CI passed except for code coverage. Here is the report. It's
all guice stuff that should be tested by the integration tests. Do people mind
if I add suppressions for these?
>
> ```
> Diff coverage statistics:
>
>
------------------------------------------------------------------------------
> | lines | branches | functions | path
>
------------------------------------------------------------------------------
> | 100% (2/2) | 100% (0/0) | 100% (1/1) |
org/apache/druid/server/initialization/jetty/JettyServerModule.java
> | 0% (0/1) | 100% (0/0) | 100% (0/0) |
org/apache/druid/server/initialization/jetty/CliIndexerServerModule.java
> | 0% (0/1) | 100% (0/0) | 100% (1/1) |
org/apache/druid/server/initialization/jetty/ChatHandlerServerModule.java
> | 0% (0/11) | 0% (0/2) | 60% (3/5) |
org/apache/druid/guice/http/JettyHttpClientModule.java
> | 72% (8/11) | 0% (0/2) | 33% (2/6) |
org/apache/druid/guice/http/HttpClientModule.java
> | 100% (1/1) | 100% (0/0) | 50% (1/2) |
org/apache/druid/guice/http/AbstractHttpClientProvider.java
>
------------------------------------------------------------------------------
>
> Total diff coverage:
> - lines: 40% (11/27)
> - branches: 0% (0/4)
> - functions: 53% (8/15)
>
> ERROR: Insufficient line coverage of 40% (11/27). Required 50%.
> ERROR: Insufficient branch coverage of 0% (0/4). Required 50%.
> ```
These suppressions seem reasonable to me. The only thing I'm slightly
concerned about is there is some logic in the Modules you listed that bind
different things based on some input. I think that level of detailed testing
would be too hard in the integration tests, but a lot easier to enforce in unit
tests. For example, in CliIndexerServerModule:
```
// Use an equal number of threads for chat handler and non-chat handler
requests.
int serverHttpNumThreads;
if (properties.getProperty(SERVER_HTTP_NUM_THREADS_PROPERTY) == null) {
serverHttpNumThreads = ServerConfig.getDefaultNumThreads();
} else {
serverHttpNumThreads =
Integer.parseInt(properties.getProperty(SERVER_HTTP_NUM_THREADS_PROPERTY));
}
```
Validating this behavior would be a lot easier in unit tests than
integration tests. However, there are other parts of the module that aren't
very unit testable, like validating a QoS filter was bound correctly
```
JettyBindings.addQosFilter(
binder,
"/druid/worker/v1/chat/*",
serverHttpNumThreads
);
```
Given this, I think it's a reasonable tradeoff to add suppressions for these
classes since the module code itself is simple to follow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
