feat: enable on-demand Toxiproxy usage in integration tests

[robfrank]

No description provided.

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage ∅ diff coverage · -7.89% coverage variation

Metric	Results
Coverage variation	✅ -7.89% coverage variation
Diff coverage	✅ ∅ diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (e50d5fd)	120069	87833	73.15%
Head commit (c5afc1f)	151308 (+31239)	98749 (+10916)	65.26% (-7.89%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4013)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review

Note: This PR is already merged, but here is a retrospective review for the record.

Overview

This PR refactors ContainersTestTemplate to start the Toxiproxy container only when needed, using a Template Method pattern hook (useToxiproxy()). NetworkDelayIT and PacketLossIT override it to opt in; all other subclasses get the default false and skip the Toxiproxy container entirely. The motivation - reducing CI resource usage and eliminating spurious failures on constrained runners - is sound.

---

What Works Well

• Template Method pattern is well-applied. The protected boolean useToxiproxy() hook is the correct abstraction: the base class controls lifecycle, subclasses declare intent. This is exactly the right design for optional infrastructure.
• Null safety in tearDown. The if (toxiproxy != null) guard correctly mirrors the conditional start, so cleanup never throws when the container was not started.
• Performance gain is real. Skipping an unnecessary container per test run reduces startup latency and Docker image pulls on CI runners that do not need fault injection.

---

Issues and Suggestions

1. Multi-line Javadoc block on a method that is self-explanatory

Per the project's CLAUDE.md conventions, comments should be at most one short line, and only when the why is non-obvious. The added Javadoc is three lines long and largely re-states what the method name and return value already communicate. A single line (or no comment at all) would be consistent with the project style.

2. Double blank lines

There are extra blank lines after the closing } of the if (useToxiproxy()) block in setupContainers() and after the overriding method bodies in both NetworkDelayIT and PacketLossIT. These are minor but inconsistent with the surrounding code style.

3. Silent NPE risk if toxiproxyClient is used without opting in

If a future subclass calls toxiproxyClient.createProxy(...) without overriding useToxiproxy(), it will fail with a NullPointerException at runtime rather than a clear error at setup time. Consider adding a guard helper:

protected ToxiproxyClient requireToxiproxyClient() {
  if (toxiproxyClient == null)
    throw new IllegalStateException("useToxiproxy() must return true to access toxiproxyClient");
  return toxiproxyClient;
}

Subclasses would call requireToxiproxyClient() instead of accessing the field directly, making misuse fail with a meaningful message rather than a cryptic NPE.

---

Summary

The change is correct, safe, and a clear improvement. The main suggestions are cosmetic (comment length, blank lines) and one forward-looking defensive measure for the toxiproxyClient field. No blocking issues.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.22%. Comparing base (e50d5fd) to head (c5afc1f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4013      +/-   ##
==========================================
- Coverage   64.24%   64.22%   -0.02%     
==========================================
  Files        1597     1597              
  Lines      120069   120069              
  Branches    25557    25557              
==========================================
- Hits        77137    77120      -17     
- Misses      32266    32278      +12     
- Partials    10666    10671       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.