fix: don't report negative timings in TCP ping

[MartinKolarik]

Related to jsdelivr/globalping#686

[coderabbitai]

Walkthrough

The PR refactors TCP ping probe types by introducing a base probe type and distinct success/failure variants: successful probes include rtt and success: true, failed probes include success: false and omit rtt. A union type TcpPingProbeData replaces the previous single probe shape and TcpPingData now references this union. Implementation changes ensure rtt is emitted only for successful probes and that timing aggregations/filtering consider only successful probes. Unit tests were updated to guard assertions on probe fields with success checks and to verify absence of rtt on failures.

Poem

🧭 Two probes diverge in typed array,

success carries time, failures keep gray,
* RTT sings only when connections stay,
* Tests now check the truth before they play,
🛠️ small changes keeping types at bay

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main fix: preventing negative timings from being reported in TCP ping functionality, which aligns with the refactoring that removes RTT from failed probes.
Description check	✅ Passed	The description references a related GitHub issue, providing context for the change, though minimal detail is given about the actual implementation.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch tcp-t

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/command/handlers/ping/tcp-ping.ts (1)

245-247: Consider explicit type predicate for improved type safety.

While TypeScript should narrow the type correctly after filtering by t.success, using an explicit type predicate can make the type narrowing more robust:

-const timings = probeData.filter(t => t.success).map(probe => ({
+const timings = probeData.filter((t): t is TcpPingSuccessProbeData => t.success).map(probe => ({
	rtt: roundNumber(probe.rtt, 2),
})).filter(t => !Number.isNaN(t.rtt));

This ensures TypeScript explicitly recognizes that after the filter, all items are TcpPingSuccessProbeData with the rtt property available.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dc1b14b and f3073bf.

📒 Files selected for processing (2)

• src/command/handlers/ping/tcp-ping.ts (4 hunks)
• test/unit/command/handlers/ping/tcp-ping.test.ts (3 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{vue,ts,js}

⚙️ CodeRabbit configuration file

We use Nuxt with auto imports enabled. Don't warn about missing imports.

Files:

• test/unit/command/handlers/ping/tcp-ping.test.ts
• src/command/handlers/ping/tcp-ping.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: Test code / Run e2e tests (20.x)
• GitHub Check: Test docker / Publish Docker image
• GitHub Check: Test code / Run tests (20.x)
• GitHub Check: Test code / Run tests (18.x)
• GitHub Check: Test docker / Publish Docker image
• GitHub Check: Test code / Run e2e tests (20.x)

🔇 Additional comments (9)

test/unit/command/handlers/ping/tcp-ping.test.ts (4)

160-165: LGTM - Proper type narrowing for successful probes.

The conditional check ensures RTT is only asserted when the probe succeeds, aligning with the new type structure.

---

178-183: LGTM - Consistent with success-based assertions.

---

198-198: LGTM - Verifies absence of RTT on failures.

This assertion directly validates the PR objective.

---

509-515: LGTM - Test data matches the new type structure.

src/command/handlers/ping/tcp-ping.ts (5)

24-40: LGTM - Well-structured discriminated union.

The type definitions cleanly separate successful and failed probe data, with the literal success property enabling proper type discrimination.

---

85-86: LGTM - Error handling excludes RTT.

---

90-91: LGTM - Timeout handling excludes RTT.

---

116-116: LGTM - Refined typing for success timings.

---

149-151: LGTM - Correct filtering of successful probes.

[MartinKolarik]

@alexey-yarmosh, please see if you can fix the tests. The code should be correct, TS builds it, but ts-node reports an error.

[alexey-yarmosh]

Turns out inferred type predicates require strict: true in tsconfig.json, since we are specifying strict: false in tests/tsconfig.json, predicate needs to be specified explicitly. But I'd rather disable ts-node type checking entirely, as we have tsc for that, and duplicate check isn’t worth issues like this one.

[MartinKolarik]

But I'd rather disable ts-node type checking entirely, as we have tsc for that, and duplicate check isn’t worth issues like this one.

TSC doesn't check the test code, otherwise, it would report errors too. And having the tests with no type checking at all would cause them to become a bigger and bigger mess over time, at which point I'd rather write them in plain JS. But the fix here turned out to be quite simple - strict comparison is enough to trigger the predicate.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/command/handlers/ping/tcp-ping.ts (1)

149-151: Minor: Consider consistent success checking.

Line 149 uses result.success (truthy check) while line 245 uses t.success === true (explicit check). Both work correctly, but consistency would be slightly clearer.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f3073bf and afeab73.

📒 Files selected for processing (2)

• src/command/handlers/ping/tcp-ping.ts (4 hunks)
• test/unit/command/ping-command.test.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{vue,ts,js}

⚙️ CodeRabbit configuration file

We use Nuxt with auto imports enabled. Don't warn about missing imports.

Files:

• test/unit/command/ping-command.test.ts
• src/command/handlers/ping/tcp-ping.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: Test docker / Publish Docker image
• GitHub Check: Test code / Run e2e tests (20.x)
• GitHub Check: Test docker / Publish Docker image
• GitHub Check: Test code / Run e2e tests (20.x)

🔇 Additional comments (4)

src/command/handlers/ping/tcp-ping.ts (3)

24-40: LGTM! Clean discriminated union design.

The type structure correctly ensures that rtt is only present on successful probes, directly addressing the PR objective of not reporting negative timings.

---

73-99: Correct implementation of success/failure distinction.

The function properly omits rtt from error and timeout cases, ensuring only successful connections report timing data.

---

245-247: Correct filtering logic.

The explicit success === true check properly narrows the type to TcpPingSuccessProbeData, ensuring type-safe access to rtt.

test/unit/command/ping-command.test.ts (1)

127-127: Correct type refinement.

Changing from never to unknown properly reflects that the mock can receive any options type, even if unused. This aligns with TypeScript best practices.

[github-actions]

🎉 This PR is included in version 0.41.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀