fix: logger methods , structural changes , removed pubsub , changed config structure and some minor tweaks, unit test cases #400
Conversation
…onfig structure and some minor tweaks
MayurWitsLab
requested review from
ashishkgGoogle,
poojajoshi2,
Deepa-Mulchandani and
tanyamadaan
as code owners
MayurWitsLab
changed the title
cmd/clientSideHandler/config.yaml
Outdated
There was a problem hiding this comment.
As I commented earlier move this to configs folder as per the shared folder structure
|
|
||
| import ( | ||
| shared "beckn-onix/shared" | ||
| "context" |
There was a problem hiding this comment.
please read up on grouping imports,
We should divide the imports into following groups :
- Standard library packages
- BecknOnix packages
- ThirdParty Packages
4 Renamed Packages
5 blank imports
Read up on this https://blog.devgenius.io/sort-go-imports-acb76224dfa7
And make the required changes everywhere else as well.
cmd/clientSideHandler/main.go
Outdated
|
|
||
| type Config struct { | ||
| AppName string `yaml:"app_name"` | ||
| ServerPort int `yaml:"port"` |
There was a problem hiding this comment.
this should be just port
cmd/clientSideHandler/main.go
Outdated
|
|
||
|
|
||
| type Config struct { | ||
| AppName string `yaml:"app_name"` |
There was a problem hiding this comment.
when using yaml naming convention is governed by surrounding code. Since we are using golang lets stick to camelcase
cmd/clientSideHandler/main.go
Outdated
| ) | ||
|
|
||
|
|
||
| type Config struct { |
There was a problem hiding this comment.
why is this exported ?
| ) | ||
|
|
||
| // TestInitConfig checks if the InitConfig function correctly reads a YAML file | ||
| func TestInitConfig(t *testing.T) { |
There was a problem hiding this comment.
Please split all test into 2 separate tests, 1 for success and one for error case, also use table driven tests,
https://golang.cafe/blog/golang-table-test-example
| t.Fatalf("InitConfig failed: %v", err) | ||
| } | ||
|
|
||
| // Validate results |
There was a problem hiding this comment.
This is not the right way for testing this, as we will have to update this every time we add a new field to config, instead of doing this create and expected config and do a diff agains it, use this package https://pkg.go.dev/github.com/google/go-cmp/cmp
| if err != nil { | ||
| http.Error(w, "Error reading body", http.StatusInternalServerError) | ||
| return | ||
| } |
There was a problem hiding this comment.
this is not needed
| } | ||
|
|
||
| if string(body) != `{"message": "Hello"}` { | ||
| t.Errorf("Expected body: %s, got: %s", `{"message": "Hello"}`, string(body)) |
There was a problem hiding this comment.
why are we testing the request. ?
| // Validate response body | ||
| expectedResponse := "Message received successfully" | ||
| if rec.Body.String() != expectedResponse { | ||
| t.Errorf("Expected response body: %q, got: %q", expectedResponse, rec.Body.String()) |
There was a problem hiding this comment.
As I have commented on the main file making the handler method independent will make all this much simpler.
fix: logger methods , structural changes , removed pubsub , changed config structure and some minor tweaks