bharathgunapati commented on PR #50:
URL:
https://github.com/apache/flink-connector-http/pull/50#issuecomment-4856334424
Thanks so much for the thorough review, David. Went through all three points:
1. archunit-junit5-engine / silently-skipped tests
I dug into this carefully and I don't think the tests are being skipped —
the engine is on the test classpath, transitively via
flink-architecture-tests-test → archunit-junit5 → archunit-junit5-engine:
+- com.tngtech.archunit:archunit:1.4.1:test
+- com.tngtech.archunit:archunit-junit5-api:1.4.1:test
\- com.tngtech.archunit:archunit-junit5:1.4.1:test
\- com.tngtech.archunit:archunit-junit5-engine:1.4.1:test
\- com.tngtech.archunit:archunit-junit5-engine-api:1.4.1:test
And the rules do execute — Surefire reports Tests run: 10 for
ProductionCodeArchitectureTest and Tests run: 2 for TestCodeArchitectureTest
(non-zero counts; a missing engine would give Tests run: 0). To make this
un-ambiguous, I've also un-frozen the @VisibleForTesting rule below (see point
3) — the build now goes red on it, which is a direct, in-CI demonstration that
the engine is running rather than skipping.
On the suggested swap to the archunit-junit5 bundle: I'd actually lean
against it, because it would re-break the maven-dependency-plugin:analyze gate.
Our test classes directly reference
com.tngtech.archunit.core.importer.ImportOption (from archunit) and the
@AnalyzeClasses/@ArchTest annotations (from archunit-junit5-api), and analyze
requires the artifacts that contain the used classes to be declared directly.
Declaring only the bundle would flag archunit + archunit-junit5-api as "used
undeclared" and the bundle as "unused declared." This mirrors what
flink-connector-kafka does (declare archunit + archunit-junit5-api, engine
transitive), so I've kept it consistent with that.
2. 56 non-public Flink internals
Agreed — freezing is the pragmatic way to land the gate, and these deserve
follow-up JIRAs. Most are the shaded-Jackson usage in JavaNetHttpPollingClient,
OidcAccessTokenManager, and GenericJsonAndUrlQueryCreator (no public JSON API
to migrate to today), plus a few internal table utilities.
3. @VisibleForTesting call from production
Fully agree this one is a genuine smell that should be fixed, not frozen.
Small clarification on the earlier state: it wasn't rule-suppressed — the rule
always ran; the single violation was just recorded in the freeze baseline. I've
now done two things:
Un-froze the rule by removing its entry from the baseline, so
NO_CALLS_TO_VISIBLE_FOR_TESTING_METHODS is a hard, enforced gate again (with
the annotation still present, the build failed on it — which also confirms the
ArchUnit engine is genuinely executing these rules, per point 1).
Applied the fix: LookupRow.getLookupEntries() is a genuine production
accessor now (used in the lookup() hot path), so the @VisibleForTesting
annotation was simply misleading. I dropped the annotation — it stays
package-private in the same package, so no visibility change — and the rule is
back to green on the fixed code with an empty baseline (not frozen).
Proof from the CI logs:
<img width="1056" height="574" alt="Screenshot 2026-07-01 at 7 53 04 PM"
src="https://github.com/user-attachments/assets/6f6a7396-b67a-4977-bf0e-efeb99fd1adb";
/>
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
