-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
fix: the logic to compare two slices should be changed #1100
fix: the logic to compare two slices should be changed #1100
Conversation
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
…arameters-in-the-response
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
…arameters-in-the-response
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
…arameters-in-the-response
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
…arameters-in-the-response
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
…obal-noisy-parameters-in-the-response' into feat-add-support-to-configure-global-noisy-parameters-in-the-response
…arameters-in-the-response
…arameters-in-the-response
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
…obal-noisy-parameters-in-the-response' into feat-add-support-to-configure-global-noisy-parameters-in-the-response
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@charankamarapu I Have made the changes |
@charankamarapu I believe code snippets in https://keploy.io/docs/running-keploy/configuration-file/ need an update. Should I create an issue ? |
what change is needed there? |
global noise field in yaml is now changed from string. So the value of the globalnoise field needs an update |
Currenly this is present in the docs, what will it be changed to ..?
|
Also some new fields are/will be added in keploy-config. |
Who ever adds the new fields will change the doc too. Have you added any new field..? |
Got it. The key strings will not be there. Please go head and create the issue but create the PR only after this PR is merged. |
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
body: {} | ||
header: {} | ||
test-sets: | ||
test-set-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.
I think we shouldn't give this test-set-name in the default config because people will just run it and keploy thinks test-set-name is name of the test set and it will search for test-set-name. Just write test-sets: and leave it . Above the globalNoise: write a comment that to use global noise please follow the guide at the end of the yaml.
CoverageReportPath string `json:"coverageReportPath" yaml:"coverageReportPath"` // directory path to store the coverage files | ||
} | ||
|
||
type Globalnoise struct { |
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.
Have you checked the case where I dont give Testsets noise at all..? like this :
globalNoise:
global:
body: {}
header: {}
If I don't give test-sets noise at all will it throw an error while converting from yaml to go struct.
In the same way if I just include this without header json will it throw an error or ignore it.
globalNoise:
global:
body: {}
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.
I have tested this from my end it is working fine. Please resolve the other comment.
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@charankamarapu I have made the changes |
hey @AkashKumar7902 , can you check why the github action is failing mostly because of changes in config file. Mostly due to backward compatability it was unable to detect noise I guess, can you check once.? |
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@AkashKumar7902 Is the PR ready for review or are there any changes..? |
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
global: | ||
body: {} | ||
header: {} | ||
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.
may be just remove this test-sets param from here, people can understand how to use it from comments below.
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 !
Signed-off-by: Akash Kumar <meakash7902@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.
Ship it!
Related Issue
Closes: #1099
Describe the changes you've made
changed the comparison logic to compare two slices in jsonMatch() function.
Type of change
Please let us know if any test cases are added
Please describe the tests(if any). Provide instructions how its affecting the coverage.
Describe if there is any unusual behaviour of your code(Write
NA
if there isn't)A clear and concise description of it.
Checklist:
Screenshots (if any)