Skip to content

Remove repeated tests from files skipped by the first pass - #357

Merged
nineinchnick merged 3 commits into
mainfrom
drop-redundant-tests-2
Oct 24, 2026
Merged

nineinchnick merged 3 commits into
mainfrom
drop-redundant-tests-2

Conversation

@nineinchnick

Copy link
Copy Markdown
Owner

This is the second pass of #355. That pass skipped the test files that open PRs were changing. Those PRs are merged now, so this pass reviews those files: TestOpenApiSpec, TestOpenApiConfig, TestOpenApiClientRequests, TestOpenApiClient, TestOpenApiServers, TestSpecLocation, TestCachingHttpClient, TestAuthenticatingHttpClient, and TestKeycloakAuthentication. It also reviews test files added since then, like TestUserCredentials and TestOpenApiSchemaFromTags. A repeated test is more code to keep in sync, and it covers nothing new. Repeated loads of a big spec make the suite slower for no gain.

Changes

Change Why
TestOpenApiSpec: the five getDatadog* tests share one parsed spec Each test parsed specs/datadog.yaml with the same config. The five tests took 4.6 s before and 1.6 s after. The trimmed-prefix test still parses its own spec, because it uses a different config
TestOpenApiConfig.testApiKeysCombinedWith*: four tests become one They checked one validation and differed only in the property name and value. The setter order test stays separate, because it checks isApiKeysValid() directly
Removed TestOpenApiClientRequests.testLeadingPathParameter testNotFoundWithPathParametersIsEmpty reads the same table of the same spec, with the same path parameter, and checks the same request URI and empty result

Coverage of src/main (JaCoCo, mvnd clean verify -Dair.check.skip-jacoco=false)

Counter Before After
Lines 4981 / 5276 4982 / 5276
Branches 2388 / 2787 2389 / 2787
Instructions 27858 / 29515 27864 / 29515

I compared the covered lines and branches one by one. No line or branch lost coverage. The one gained line is ProbeLimit.toString() with a set limit (line 52). As #355 found, that line runs only as the session property encoder, so its coverage depends on timing.

Tests: 485 before, 481 after.

Not touched

Open PR #356 changes authentication/TestAuthentication.java, so I left it alone.

Possibly redundant, but kept

  • TestCachingHttpClient.testRequestCanForceRevalidation: testCacheDirectivesDontSelectStoredResponse sends the same first two requests and makes the same final assertions. But it checks only the state after a third Pragma request, so it can't tell which request revalidated.
  • TestCachingHttpClient.testCookieParametersSelectStoredResponse: HttpCache puts every header in the key, so this runs the same code as testHeaderParametersSelectStoredResponse. It still protects against Cookie being left out of the key later.
  • TestAuthenticatingHttpClient.testRejectedTokenOfClientSecretBasicIsRefreshed: its assertions are a subset of testRejectedTokenIsRefreshed. But it is the only fast test of a refresh with client_secret_basic. The Keycloak test needs a container.
  • TestCachingHttpClient.testVary: every header is already in the cache key, so its assertions would still pass if Vary matching broke. A stronger test would be better than removing it.
  • TestOpenApiSpec.testResultsPathToArrayOrObject and testResultsPathToUnion: their first assertions repeat testPaginatedResultsNullability and testUnionOfObjectAndArrayResponses. Their other assertions are unique.
  • TestOpenApiSpec.testTablesWithoutColumns: its /names part repeats testPlainValueResponses. The /count part is unique.
  • TestOpenApiSpec.getGithubPaginatedTables and getGithubPatchedOperations overlap, but each checks things the other doesn't.
  • TestOpenApiConfig.testTrimmedPathPrefixes: its first assertion repeats testExplicitPropertyMappings. Its error message is unique.
  • TestOpenApiClientRequests.testInvalidJsonIsExternalError and testReadsResponsesWithLoggedBodies have the same message, but the second one runs with body logging on.

The bundled Datadog spec is big, and parsing it takes most of a second.
Five tests parsed it again with the same configuration, so they now
share one parsed spec, which saves about 4 seconds of the test run.
Four tests checked the same validation, which rejects combining
authentication.api-keys with any of the single API key properties.
They differed only in the property name and value, so one test with a
line per property shows the whole rule at once.
testNotFoundWithPathParametersIsEmpty reads the same table of the same
spec, with the same path parameter, and checks the same request URI and
empty result, so testLeadingPathParameter covered nothing more.
@nineinchnick
nineinchnick merged commit e96c329 into main Oct 24, 2026
1 check passed
@nineinchnick
nineinchnick deleted the drop-redundant-tests-2 branch October 24, 2026 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant