Handle error for 0 doc hits for a data stream #744
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
|
@r00tu53r Could you please fix the linting issues? |
| // no hits were found for a data stream | ||
| if len(docs) == 0 { | ||
| multiErr = append(multiErr, fmt.Errorf("no documents found in %s data stream", dataStream)) | ||
| return multiErr |
There was a problem hiding this comment.
I think that to make it correct, the code execution must reach line 676 (return testrunner.ErrTestCaseFailed{) to convert the multiErr.
I would leave the multiErr = append(...) in line 657, but wrap for-loop with if-clause.
There was a problem hiding this comment.
Thanks for pointing that out @mtojek. Sorry missed that. I've made the change. I hope I understood it right.
| // prevents panic and performs cleanup/teardown for cases where | ||
| // no hits were found for a data stream | ||
| if len(docs) == 0 { | ||
| multiErr = append(multiErr, fmt.Errorf("no documents found in %s data stream", dataStream)) |
There was a problem hiding this comment.
Hmmm.. I'm trying to deduce what should be the logic here. Function validateFields checks if documents are valid according to the schema loaded to the FieldsValidator, so the question is how should it work if there are no documents passed?
Maybe it's possible to fail fast sooner and not call the validateFields?
WDYT?
cc @jsoriano
There was a problem hiding this comment.
Hmm .. What I thought is - In this case where we run system tests if there are no documents passed it means there is an error in the pipeline. We have the data for sure in the files but it didn't match the conditions of the pipeline.
I'll have another look at the logic from a few calls above the call stack to understand how else this can be handled. Thanks
There was a problem hiding this comment.
Hmm .. What I thought is - In this case where we run system tests if there are no documents passed it means there is an error in the pipeline
My understanding is that if we can don't have any documents, we shouldn't call the validation of documents :)
There was a problem hiding this comment.
My understanding is that if we can don't have any documents, we shouldn't call the validation of documents :)
Thanks :). I think I see what you mean. I agree validateFields needn't be invoked. The runner invokes tests per each variant. For a case where there were no doc hits if an error is returned then it would not run tests for other variants. As you see here - https://github.com/elastic/elastic-package/blob/main/internal/testrunner/runners/system/runner.go#L176-L179. Do you think that's acceptable behaviour?
I've modified the code.
There was a problem hiding this comment.
Yes, it's much shorter now :) If it fixes the panic, then it's 👍 . Thank you for the fix, Sai!
3 participants
Return error for system test cases where zero or no doc hits were found on a particular data stream. Without this error check the code panics and prevents proper cleanup of the system test containers which prevents subsequent system test runs.
Error before this fix:
Messages after the fix: