-
-
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 unable to record custom name test cases #1189
fix unable to record custom name test cases #1189
Conversation
Apply Sweep Rules to your PR?
|
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.
Please resolve the comments
pkg/platform/yaml/yaml.go
Outdated
@@ -68,7 +68,7 @@ func findLastIndex(path string, Logger *zap.Logger) (int, error) { | |||
fileNameWithoutExt := fileName[:len(fileName)-len(filepath.Ext(fileName))] | |||
if len(strings.Split(fileNameWithoutExt, "-")) < 2 { | |||
Logger.Error("failed to decode the last sequence number from yaml test", zap.Any("for the file", fileName), zap.Any("at path", 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.
error log should also be removed right because we don't care what is the name of the file if it match "test-x" then only we should count it.
@@ -68,7 +68,7 @@ func findLastIndex(path string, Logger *zap.Logger) (int, error) { | |||
fileNameWithoutExt := fileName[:len(fileName)-len(filepath.Ext(fileName))] | |||
if len(strings.Split(fileNameWithoutExt, "-")) < 2 { | |||
Logger.Error("failed to decode the last sequence number from yaml test", zap.Any("for the file", fileName), zap.Any("at path", path)) | |||
return 0, errors.New("failed to decode the last sequence number from yaml test") | |||
continue | |||
} | |||
indxStr := strings.Split(fileNameWithoutExt, "-")[1] | |||
indx, err := strconv.Atoi(indxStr) |
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.
even for this error we shouldn't throw any log or return error.
if tc.Name == "" { | ||
// finds the recently generated testcase to derive the sequence number for the current testcase |
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 test all the cases once for this feature.
- record tests normally and run test mode. Don't use this feature and don't change any test case name.
- record tests normally and record using this feature somewhere in the middle and run in test mode.
- Along with second case rename test case manually in between recording and run in test mode.
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 are the test-reports for all the specified scenarios:
in the 3rd case, if we just rename the test case file and if we try running only that particular file (refer PR 1165
), then it will result in an error:
🐰 Keploy: 2023-12-12T17:59:27+05:30 ERROR test/test.go:465 failed to fetch test results {"error": "🐰 Keploy:%!(EXTRA string=found no test results for test report with id: %v, string=report-24)"}
we need to go inside the test case file and change the name parameter as well.
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 does this happen any analysis..? I mean we just get the file and run the content right. Do we do any validation like checking if parameter in the file is equal to file 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.
in line 555 of test.go, we are iterating through initialisedValues.Tcs
which contains information about the testcase from inside the files. so to run a particular testcase, the value of the name field present in the files will be compared to the testcase name you provided. renaming alone won't work as the comparison is done with the name field not the filename.
the error will arise if there are several testcases and you want to run something that does not match with the name of any testcase present. This error is caused because in FetchTestReport
, the error condition (passed + failed == 0) is true, as no testcase ran.
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.
Through this feature like renaming using header we will change the tc.name also right along with file name...? leave about renaming manually we can skip it.
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
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Is the PR ready for review..? |
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
if tc.Name == "" { | ||
// finds the recently generated testcase to derive the sequence number for the current testcase |
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 does this happen any analysis..? I mean we just get the file and run the content right. Do we do any validation like checking if parameter in the file is equal to file name..?
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: #1188
Describe the changes you've made
the
findlastindex
must be called only when no custom test case name is provided. so I have changed the order of function callType 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)