-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
patch: modified the Route.Run() to use APP_PORT #58
Conversation
return errors.New("port can't be empty") | ||
} | ||
completeHost := defaultHost + ":" + defaultPort | ||
host = append(host, completeHost) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Bro, can you also change the RunTLS
method like this? Then we also need to change the config/route.go
file in goravel/goravel
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I have pushed the changes, I am just try to fix failing test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @hwbrzzl can you help me with testcases, I am not able to resolve them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hwbrzzl There is some issue with testcases, I am trying to debug it, will push it by Wednesday.
route/gin_test.go
Outdated
@@ -39,6 +39,7 @@ func TestRun(t *testing.T) { | |||
name: "error when default host is empty", | |||
setup: func(host string) error { | |||
mockConfig.On("GetString", "route.host").Return(host).Once() | |||
mockConfig.On("GetString", "route.port").Return("test_ca.port").Once() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't need to add this, because we are testing host can't be empty
.
We need to add a test case: port can't be empty
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I have added it.
route/gin_test.go
Outdated
@@ -53,6 +54,7 @@ func TestRun(t *testing.T) { | |||
setup: func(host string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setup: func(host string) error { | |
setup: func(host, port string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@hwbrzzl Review changes are done, and please find PR for review. |
Codecov ReportBase: 48.35% // Head: 48.43% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #58 +/- ##
==========================================
+ Coverage 48.35% 48.43% +0.08%
==========================================
Files 102 102
Lines 6320 6330 +10
==========================================
+ Hits 3056 3066 +10
Misses 3070 3070
Partials 194 194
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
Hey @sidshrivastav We created a private channel in Discord for contributors. You can click the link and DM me (@bowen) to join it if you are interested. Here you can:
|
No description provided.