Merged
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
Reviewer's GuideExpands GeoIP, HTTP handler, and API server test coverage with integration and error-path tests, refactors server startup into a testable constructor, and introduces changelog tooling and updated lint configuration. Sequence diagram for API server startup via NewServersequenceDiagram
participant main
participant NewServer
participant geoip
participant handlers
participant echo
main->>NewServer: NewServer("data/city.db")
activate NewServer
NewServer->>geoip: NewService(dbPath)
alt GeoIP service init fails
geoip-->>NewServer: error
NewServer-->>main: nil, nil, error
main->>main: log.Fatalf("Failed to initialize GeoIP service")
else GeoIP service init succeeds
geoip-->>NewServer: geoip.Service
NewServer->>handlers: create GeoIPHandler
NewServer->>echo: echo.New()
echo-->>NewServer: *echo.Echo
NewServer-->>main: *echo.Echo, *geoip.Service, nil
main->>main: defer geoService.DB.Close()
main->>echo: e.Start(":" + port)
end
deactivate NewServer
Updated class diagram for API server construction and dependenciesclassDiagram
class main {
+NewServer(dbPath string) *echo_Echo, *geoip_Service, error
+main()
}
class echo_Echo {
+GET(path string, handler func)
+Start(address string) error
}
class geoip_Service {
+DB *geoip_DB
}
class geoip_DB {
+Close() error
}
class handlers_GeoIPHandler {
+GeoService *geoip_Service
+Lookup(c echo_Context) error
}
class echo_Context {
}
main --> geoip_Service : uses
main --> echo_Echo : constructs
main --> handlers_GeoIPHandler : constructs
handlers_GeoIPHandler --> geoip_Service : depends on
geoip_Service --> geoip_DB : owns
echo_Echo --> echo_Context : uses in handlers
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
TestJSONSerializer_Serialize, usingassert.JSONEqmeans the indented vs non-indented cases don’t actually validate theindentbehavior (whitespace is ignored); consider using direct string equality at least for the indented case to ensure formatting is exercised. - In
TestReader_ForcedExecution, the returned error from each action is explicitly discarded (_ = err), which can hide real failures; either assert on the expected error behavior or document why errors are intentionally ignored. - The repo now contains both
.golangci.ymland.golangci.bck.yml; keeping two similar config files can be confusing, so consider removing or clearly marking the backup to avoid ambiguity about which configuration is authoritative.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TestJSONSerializer_Serialize`, using `assert.JSONEq` means the indented vs non-indented cases don’t actually validate the `indent` behavior (whitespace is ignored); consider using direct string equality at least for the indented case to ensure formatting is exercised.
- In `TestReader_ForcedExecution`, the returned error from each action is explicitly discarded (`_ = err`), which can hide real failures; either assert on the expected error behavior or document why errors are intentionally ignored.
- The repo now contains both `.golangci.yml` and `.golangci.bck.yml`; keeping two similar config files can be confusing, so consider removing or clearly marking the backup to avoid ambiguity about which configuration is authoritative.
## Individual Comments
### Comment 1
<location> `.golangci.yml:7-12` </location>
<code_context>
-
-issues:
- exclude-use-default: false
+ exclusions:
+ generated: lax
+ paths:
+ - third_party$
+ - builtin$
+ - examples$
+formatters:
+ exclusions:
</code_context>
<issue_to_address>
**issue (bug_risk):** The `exclusions` keys under `linters` and `formatters` don't match golangci-lint's documented schema and may be ignored
This config shape isn’t part of the documented golangci-lint schema, so these exclusions are likely ignored and `generated`, `third_party`, `builtin`, and `examples` won’t actually be skipped. Please switch to supported fields like `run.skip-dirs`, `run.skip-files`, or `issues.exclude-rules` so the exclusions take effect as intended.
</issue_to_address>
### Comment 2
<location> `internal/geoip/service_test.go:83` </location>
<code_context>
+ name: "Valid Google DNS (US)",
+ ipStr: "8.8.8.8",
+ expectFound: true,
+ expectedISO: "US", // Usually resolves to US
+ },
+ {
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid depending on a specific country ISO for `8.8.8.8` to reduce flakiness
Hard-asserting `expectedISO: "US"` ties this test to a specific GeoIP dataset and will likely become flaky as databases/providers change. Consider either dropping the ISO assertion here or making it softer (e.g., only asserting `HasData()` or conditionally asserting ISO when it matches a known value and otherwise logging/skipping) so the test validates behavior rather than a particular dataset.
</issue_to_address>
### Comment 3
<location> `cmd/api/main_test.go:34` </location>
<code_context>
+ {
+ name: "Indented Serialization",
</code_context>
<issue_to_address>
**issue (testing):** The indented JSON case in `TestJSONSerializer_Serialize` is not actually validating indentation
Since `assert.JSONEq` ignores whitespace, this case doesn’t actually verify that `Serialize` applies the indent string; it just rechecks semantic equality already covered elsewhere. To test indentation, either compare `rec.Body.String()` to `expectedBody` with `assert.Equal`, or add a specific formatting check (for example, `assert.Contains(t, rec.Body.String(), "\n \"hello\": \"world\"")`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
gustavosett
force-pushed
the
gc-chore-increase-coverage
branch
from
b827a9a to
a1cae6e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Increase coverage
Type of change
chore
Summary by Sourcery
Increase test coverage and observability around GeoIP models, service, HTTP/API behavior, and application wiring while adding basic changelog automation.
Build:
CI:
Documentation:
Tests:
Chores: