-
-
Notifications
You must be signed in to change notification settings - Fork 420
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
refactor: Refactored the code to make it more modular #1078
refactor: Refactored the code to make it more modular #1078
Conversation
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Apply Sweep Rules to your PR?
|
@@ -19,38 +19,37 @@ import ( | |||
var Emoji = "\U0001F430" + " Keploy:" | |||
|
|||
type recorder struct { | |||
logger *zap.Logger | |||
Logger *zap.Logger |
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.
Why do we need to export logger, since it is called internally by the exported functions.
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.
reverted.
pkg/service/test/util.go
Outdated
@@ -271,6 +298,11 @@ func filterTcsMocks(tc *models.TestCase, m []*models.Mock, logger *zap.Logger) [ | |||
} | |||
|
|||
for _, mock := range m { | |||
if mock.Version == "api.keploy-enterprise.io/v1beta1" { | |||
logger.Info("This mock was recorded using the the enterprise version, may not work properly with the open source version", zap.String("mock kind:", string(mock.Kind))) |
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.
Please remove the typo here, and can the info be logged once for a test-set.
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
pkg/service/test/service.go
Outdated
@@ -11,5 +11,9 @@ import ( | |||
|
|||
type Tester interface { | |||
Test(path string, proxyPort uint32, testReportPath string, appCmd string, testsets []string, appContainer, networkName string, Delay uint64, passThorughPorts []uint, apiTimeout uint64, noiseConfig map[string]interface{}) bool | |||
RunTestSet(testSet, path, testReportPath, appCmd, appContainer, appNetwork string, delay uint64, pid uint32, ys platform.TestCaseDB, loadedHook *hooks.Hook, testReportfs yaml.TestReportFS, testRunChan chan string, apiTimeout uint64, ctx context.Context, noiseConfig map[string]interface{}) models.TestRunStatus | |||
RunTestSet(testSet, path, testReportPath, appCmd, appContainer, appNetwork string, delay uint64, pid uint32, ys platform.TestCaseDB, loadedHook *hooks.Hook, testReportfs yaml.TestReportFS, testRunChan chan string, apiTimeout uint64, ctx context.Context, noiseConfig map[string]interface{})models.TestRunStatus |
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.
Here, since the parameters of this functions are long. You can add an option struct for this functions. And for the required fields can use a check for null values.
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
pkg/service/test/util.go
Outdated
Ys platform.TestCaseDB | ||
LoadedHooks *hooks.Hook | ||
AbortStopHooksInterrupt chan bool | ||
Ok bool |
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.
In the changes ok is used for returning false for errors in starting app or loading hooks so, instead of boolean we can just return the error along with InitialiseTestReturn for the InitialiseTestReturn.
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
pkg/service/test/util.go
Outdated
AbortStopHooksForcefully bool | ||
Ps *proxy.ProxySet | ||
ExitCmd chan bool | ||
Ys platform.TestCaseDB |
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.
Here since the fields are exported, you can use descriptive name or add comment for the field
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.
Change ps, ys and dIDE
pkg/service/mocktest/mocktest.go
Outdated
@@ -21,13 +21,13 @@ import ( | |||
var Emoji = "\U0001F430" + " Keploy:" | |||
|
|||
type mockTester struct { | |||
logger *zap.Logger | |||
Logger *zap.Logger |
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.
If logger is not used externally, it could be unexported.
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
pkg/service/mockrecord/mockrecord.go
Outdated
@@ -21,13 +21,13 @@ import ( | |||
var Emoji = "\U0001F430" + " Keploy:" | |||
|
|||
type mockRecorder struct { | |||
logger *zap.Logger | |||
Logger *zap.Logger |
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.
If it is not used by the external packages, this can be unexported.
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
pkg/proxy/proxy.go
Outdated
@@ -55,6 +54,13 @@ func getNextID() int64 { | |||
return atomic.AddInt64(&idCounter, 1) | |||
} | |||
|
|||
type DepInterface interface { |
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.
Remove interface from the name of the type
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.
Modified
cmd/record.go
Outdated
@@ -19,7 +19,7 @@ func NewCmdRecord(logger *zap.Logger) *Record { | |||
recorder := record.NewRecorder(logger) | |||
return &Record{ | |||
recorder: recorder, | |||
logger: logger, | |||
Logger: logger, |
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.
If this is not used by the external packages, then it can be unexported.
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
pkg/hooks/connection/factory.go
Outdated
@@ -98,6 +98,7 @@ func (factory *Factory) GetOrCreate(connectionID structs.ConnID) *Tracker { | |||
} | |||
|
|||
func capture(db platform.TestCaseDB, req *http.Request, resp *http.Response, logger *zap.Logger, ctx context.Context, reqTimeTest time.Time, resTimeTest time.Time) { | |||
V1Beta1 := models.GetVersion() |
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.
Here, you can use version as the variable name
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
pkg/models/testcase.go
Outdated
V1Beta1 Version = Version("api.keploy.io/v1beta1") | ||
V1Beta2 Version = Version("api.keploy.io/v1beta2") | ||
var ( | ||
V1Beta1 = Version("api.keploy.io/v1beta1") |
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.
This should be a constant and to store the current version, an un-exported global scoped variable.
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
@@ -643,7 +643,7 @@ func recordMySQLMessage(h *hooks.Hook, mysqlRequests []models.MySQLRequest, mysq | |||
"responseOperation": responseOperation, | |||
} | |||
mysqlMock := &models.Mock{ | |||
Version: models.V1Beta2, | |||
Version: models.V1Beta1, |
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.
Handle for the mysql server to register it as a dependency in the proxy.
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
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.
LGTM
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Signed-off-by: Pranshu Srivastava <iampranshu24@gmail.com>
Related Issue
The code did not have many modules so it became difficult to use certain parts of it, and the same work needed to be repeated again and again.
Describe the changes you've made
Made the code more modular by dividing the code into two parts, the code which is supposed the same everywhere, and the code which is supposed to be different.
Type of change
Checklist: