test(version-tests): add tests for client, domain, and service layers by margi212 · Pull Request #570 · docling-project/docling-java

margi212 · 2026-06-27T05:28:06Z

Summary

Adds comprehensive unit test coverage across the client, domain, and service layers of the docling-version-tests module.

Changes

New Test Classes

File	Class Under Test	# Tests
`RegistryClientFactoryTests`	`RegistryClientFactory`	8
`GHCRClientLoggerTests`	`GHCRClientLogger`	16
`TagsTestRequestTests`	`TagsTestRequest`	17
`NamedThreadFactoryTests`	`NamedThreadFactory`	7
`WorkParallelizerTests`	`WorkParallelizer`	10

Extended Test Classes

File	What was added
`TagTestResultTests`	9 new tests for `TagsTestResults` sorting, `from()` factory, `toBuilder()`, `setResults()`/`clearResults()`, failure detection, and nested/long stack trace handling

Coverage Highlights

RegistryClientFactory — validates correct client dispatch for ghcr.io, plus rejection of unsupported, null, empty, whitespace-padded, and case-mismatched registry strings; verifies client instance reuse.
GHCRClientLogger — validates conditional request/response logging, masking of token fields in JSON bodies, Authorization and Set-Cookie header redaction, and graceful handling of null, empty, non-JSON, and JSON-array bodies.
TagsTestRequest — validates builder construction, toBuilder() round-trip fidelity, method chaining, override behavior, and mandatory executor field enforcement.
NamedThreadFactory — validates thread naming with incrementing suffix, isolated counters per factory instance, and thread-safe concurrent creation.
WorkParallelizer — validates parallel transform and run semantics across empty, single, multi-item, and large (100-item) workloads with type-safe generic transformation.
TagTestResult / TagsTestResults — extends existing tests with version-sorted result sets, factory construction from a TagsTestRequest, mutable builder operations, and failure presence detection.

Test Design Notes

All tests follow Arrange / Act / Assert structure with assertThat (AssertJ).
Edge cases (null, empty, whitespace) are tested explicitly to guard against NPEs and silent failures.
Concurrent tests use CountDownLatch / AtomicInteger to avoid race conditions in assertions.
No production code was modified; this PR is purely additive test coverage.

…ervice layers - Add RegistryClientFactoryTests: GHCR client resolution, unsupported/null/empty/whitespace/case-sensitive registry, instance reuse - Add GHCRClientLoggerTests: request/response logging toggle, token/header masking, null/empty/non-JSON body, redirect response - Add TagsTestRequestTests: builder construction, toBuilder(), field setters, null/empty edge cases, executor null validation - Add NamedThreadFactoryTests: named thread creation, counter increment, separate factory counters, runnable execution, concurrent creation - Add WorkParallelizerTests: parallel transform/run, empty/single/large lists, multi-type transformation, concurrent execution - Extend TagTestResultTests: TagsTestResults sorting, from() factory, toBuilder(), setResults()/clearResults(), failure detection Signed-off-by: Margi <39212098+margi212@users.noreply.github.com>

github-actions · 2026-07-07T13:14:55Z

:java_duke: JaCoCo coverage report

Overall Project	47.33%	🔴

There is no coverage information present for the Files changed

github-actions · 2026-07-07T13:14:57Z

	Tests	Passed ✅	Skipped	Failed
Gradle Test Results (all modules & JDKs)	1840 ran	1840 passed	0 skipped	0 failed

Test	Result
No test annotations available

View Gradle Test Results (all modules & JDKs)

github-actions · 2026-07-07T13:14:58Z

HTML test reports are available as workflow artifacts (zipped HTML).

• Download: Artifacts for this run

edeandrea

Hi @margi212 Thanks for doing this! I've made some specific comments in certain places but I'll add some general comments here.

You may be new to AssertJ and thats totally fine, but if possible could you try to keep the assertions more fluent? Thats one of the main powers of AssertJ - rather than having lots of single assertThat statements, assertions can be combined in a fluent style.

The other thing I noticed was that there were lots of tests that were just variations on the same test. Many of them are unneeded, and others could be re-written using parameterized tests (same test assertion(s) just with different inputs).

edeandrea · 2026-07-07T13:02:09Z

+    }
+
+    @Test
+    void shouldRunItemsInParallel() {


I'm not sure what the purpose of this test is or what its trying to prove?

edeandrea · 2026-07-07T13:02:46Z

+
+    @Test
+    void shouldRunEmptyList() {
+        var executor = Executors.newSingleThreadExecutor();


I'm not sure what the purpose of this test is or what its trying to prove?

edeandrea · 2026-07-07T13:03:00Z

+    }
+
+    @Test
+    void shouldRunSingleItem() {


I'm not sure what the purpose of this test is or what its trying to prove?

edeandrea · 2026-07-07T13:04:10Z

+
+import org.junit.jupiter.api.Test;
+
+class WorkParallelizerTests {


I feel like there are many overlapping tests here that aren't needed and don't really prove anything. The first two which do something on a non-empty & empty list are probably enough. The rest of them are really different variations of the same test.

edeandrea · 2026-07-07T13:05:25Z

+
+import org.junit.jupiter.api.Test;
+
+class NamedThreadFactoryTests {


I feel like there are many overlapping tests here that aren't needed and don't really prove anything. The first one which creates a named thread and verifies the thread name is probably sufficient. The rest of them are really different variations of the same test.

edeandrea · 2026-07-07T13:09:59Z

+import io.vertx.core.http.HttpClientResponse;
+import io.vertx.core.http.HttpMethod;
+
+class GHCRClientLoggerTests {


There are a lot of repetitive tests here that are variations on the same test (response status, headers, etc).

edeandrea · 2026-07-07T13:13:06Z

+
+import ai.docling.client.tester.client.ghcr.GHCRClient;
+
+class RegistryClientFactoryTests {


I'd say the tests are all valid, but many of them are the same test just with different inputs and the same expectations.

They could be cleaned up using JUnit's ParameterizedTest - that way you only have to write the test logic once and pass different inputs into the test.

edeandrea · 2026-07-07T13:17:00Z

+        assertThat(request.image()).isEqualTo("docling-project/docling-serve");
+        assertThat(request.executor()).isEqualTo(TEST_EXECUTOR);
+        assertThat(request.cleanupContainerImages()).isTrue();
+        assertThat(request.tags()).containsExactly("v1.0.0", "v1.1.0");


Rather than a handful of individual asserts like this you could do something like this instead:

var request = ... // Build the request using the builder as you have var expected = new TagsTestRequest("ghcr.io", "docling-project/docling-serve", TEST_EXECUTOR, true, List.of("v.1.0.0", "v1.1.0")); assertThat(request) .isNotNull() .usingRecursiveComparison() .isEqualTo(expected);

AssertJ provides a nice way to perform equality by doing field-by-field comparisons (see https://assertj.github.io/doc/#assertj-core-recursive-comparison).

edeandrea · 2026-07-07T13:18:46Z

+                .tags(List.of("v2.0.0"))
+                .build();
+
+        assertThat(originalRequest.registry()).isEqualTo("ghcr.io");


Similar comment as above regarding the usage of recursive comparisons

edeandrea · 2026-07-07T13:19:36Z

+        assertThat(copy.cleanupContainerImages()).isEqualTo(original.cleanupContainerImages());
+        assertThat(copy.tags()).isEqualTo(original.tags());
+    }
+


The rest of these tests are just variants of the same tests and aren't needed.

margi212 added 2 commits June 27, 2026 10:55

Merge branch 'main' into test--add-test-cases

479c3af

margi212 changed the title ~~Adds comprehensive unit test coverage across the client, domain, and service layers of the docling-version-tests module.~~ test: Adds comprehensive unit test coverage across the docling-version-tests module. Jun 29, 2026

margi212 changed the title ~~test: Adds comprehensive unit test coverage across the docling-version-tests module.~~ test(version-tests): add tests for client, domain, and service layers Jun 29, 2026

edeandrea requested changes Jul 7, 2026

View reviewed changes


		import org.junit.jupiter.api.Test;

		class WorkParallelizerTests {


		import org.junit.jupiter.api.Test;

		class NamedThreadFactoryTests {


		import ai.docling.client.tester.client.ghcr.GHCRClient;

		class RegistryClientFactoryTests {

Uh oh!

Conversation

margi212 commented Jun 27, 2026

Summary

Changes

New Test Classes

Extended Test Classes

Coverage Highlights

Test Design Notes

Uh oh!

github-actions Bot commented Jul 7, 2026

:java_duke: JaCoCo coverage report

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

edeandrea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants