feat: add more dc networks#849
Conversation
WalkthroughThis pull request adds detection overrides for two hosting providers: Hetzner Online and OVH. The Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/lib/geoip/overrides.tsOops! Something went wrong! :( ESLint: 10.4.0 SyntaxError: Unexpected token ':' test/tests/unit/geoip/client.test.tsOops! Something went wrong! :( ESLint: 10.4.0 SyntaxError: Unexpected token ':' Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/tests/unit/geoip/client.test.ts (1)
228-274: ⚡ Quick winAdd a dedicated OVH override test case.
These updates validate Hetzner override behavior, but the new OVH override path is still untested. Add one
lookup()scenario withnormalizedNetworkmatching OVH and assertisHosting: true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tests/unit/geoip/client.test.ts` around lines 228 - 274, Add a new unit test alongside the existing Hetzner cases that calls client.lookup(MOCK_IP) with nockGeoIpProviders configured to return a city and network that normalize to OVH (use the same pattern as the Hetzner tests but set the provider response so normalizedNetwork becomes 'ovh' or matches the OVH override), then assert the returned object has normalizedNetwork: 'ovh' (or the exact normalized string your override expects) and isHosting: true; locate the relevant test block near the other lookup tests and mirror their structure (use client.lookup, MOCK_IP, and expect(info).to.deep.equal({...})) so the OVH override path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/tests/unit/geoip/client.test.ts`:
- Around line 228-274: Add a new unit test alongside the existing Hetzner cases
that calls client.lookup(MOCK_IP) with nockGeoIpProviders configured to return a
city and network that normalize to OVH (use the same pattern as the Hetzner
tests but set the provider response so normalizedNetwork becomes 'ovh' or
matches the OVH override), then assert the returned object has
normalizedNetwork: 'ovh' (or the exact normalized string your override expects)
and isHosting: true; locate the relevant test block near the other lookup tests
and mirror their structure (use client.lookup, MOCK_IP, and
expect(info).to.deep.equal({...})) so the OVH override path is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d393fbb-ce41-434b-b42f-c186ed120451
📒 Files selected for processing (2)
src/lib/geoip/overrides.tstest/tests/unit/geoip/client.test.ts
|
So are we sure these were mistagged and it wasn't a matching issue? Because I didn't see such probes at least in the current probe list. |
Adding overrides so OVH or Hetzner probes are not marked as eyeball.
Also Baxet uses both
BaxetandBaxet Groupnetwork name. It might make sense to either merge them, or make regex broader/baxet group/ -> /baxet/.