-
-
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
[Feature]: Create a normalisation command in keploy CLI #1589
Conversation
Signed-off-by: Akash <akashsingh2210670@gmail.com>
Signed-off-by: Akash <akashsingh2210670@gmail.com>
…e doesnt match yaml file Signed-off-by: Akash <akashsingh2210670@gmail.com>
Signed-off-by: Akash <akashsingh2210670@gmail.com>
@charankamarapu Kindly review this PR and let me know if this implementation works! |
From the screenshot I can say there is a bit of feature missing. User should give the testcases and testSet name which user want to normalise. |
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 address the comment
Alright, if that is how we would want to implement this feature it should not be a problem at all. Will update asap and revert! |
Signed-off-by: Akash <akashsingh2210670@gmail.com>
@charankamarapu Let me know if this implementation is ready to go or there is a need to further include more features in this! |
Signed-off-by: Akash <akashsingh2210670@gmail.com>
Signed-off-by: Akash <akashsingh2210670@gmail.com>
Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>
@charankamarapu If the CI pipeline will require time to fix, this PR is ready to be merged |
cool will do a review and let you know. |
Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>
@charankamarapu I have addressed the review comments. Looking forward to your review on them |
Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>
Signed-off-by: Akash Singh <akashsingh2210670@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.
please address the comments
@@ -155,6 +155,10 @@ func (c *CmdConfigurator) AddFlags(cmd *cobra.Command, cfg *config.Config) error | |||
switch cmd.Name() { | |||
case "update": | |||
return nil | |||
case "normalise": |
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.
Actually we should give option to the user to provide the test-run also so the flags would test-run, test-set, test-case. regarding path flag it should be normal path flag which we get in record and replay commands. It shouldn't be specific to reports. We have a config file which will be used for provided info like path etc without flags. There is a package for it named config.
cli/provider/cmd.go
Outdated
@@ -377,6 +381,35 @@ func (c CmdConfigurator) ValidateFlags(ctx context.Context, cmd *cobra.Command) | |||
} | |||
} | |||
} | |||
case "Normalise": | |||
path := viper.GetString("path") |
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.
You need not do this if add things in config please go through test and record command
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.
Refactored
pkg/service/tools/tools.go
Outdated
@@ -283,3 +287,113 @@ func (t *Tools) CreateConfig(_ context.Context, filePath string, configData stri | |||
t.logger.Info("Config file generated successfully") | |||
return nil | |||
} | |||
|
|||
// Normalise initiates the normalise process for normalising the test cases. | |||
func (t *Tools) Normalise(_ context.Context) 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.
You should use yaml functions to edit the test-cases. You should how it is done in record and replay via interfaces of DBs . you can add this function in record or replay service.
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.
Added it to replay service
Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>
Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>
Signed-off-by: Akash Singh <akashsingh2210670@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.
Please address the comments
// Normalise initiates the normalise process for normalising the test cases. | ||
func (r *Replayer) Normalise(_ context.Context, cfg *config.Config) error { | ||
path := cfg.Path | ||
testSets := viper.GetString("test-sets") |
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.
These are not needed use config struct instead of getting them like this. Please refer to how test or record command are accessing these kind of flags
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.
Will look into this
if strings.HasSuffix(file.Name(), ".yaml") { | ||
filePath := filepath.Join(testRunPath, file.Name()) | ||
// Read the YAML file | ||
yamlData, err := os.ReadFile(filePath) |
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 are you not using the test or report dbs..?
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.
Will look into this
@Akash-Singh04 if the comments are resolved feel free to resolve them, @charankamarapu would review the updated PR |
@shivamsouravjha Need some time to properly look into the refactored codebase to resolve the comments, |
Sure thing @Akash-Singh04 in the meanwhile you can move it as a draft, |
Related Issue
Closes: #1538
Describe the changes you've made
Added
keploy normalise
command to the Keploy CLI to allow users to normalise their test cases. The Function of the command is as follows:test-sets
,test-cases
andtest-run
flags from the CLIType of change
Please let us know if any test cases are added
NIL
Describe if there is any unusual behaviour of your code(Write
NA
if there isn't)NA
Checklist:
Screenshots (if any)