tests(health api): extract specific assertions from list validator#18937
tests(health api): extract specific assertions from list validator#18937
Conversation
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label. Could you fix it @yaauie? 🙏
|
yaauie
force-pushed
the
yaauie-health-api-test-improvementws
branch
from
7c911b4 to
3641c97
Compare
yaauie
force-pushed
the
yaauie-health-api-test-improvementws
branch
from
3641c97 to
867dd60
Compare
| for result in results: | ||
| if result.get('help_url') and "health-report-pipeline-" not in result.get('help_url'): | ||
| return False | ||
| if not all(key in result and result[key] == value for key, value in expect.items()): |
There was a problem hiding this comment.
none of the current validators actually require unordered comparison, so the replacement does ordered comparison. If/when we need that, we can add an $unordered matcher like the new $include matcher to do the unordered matching.
| diagnosis: | ||
| - cause: "pipeline is not running, likely because it has encountered an error" | ||
| action: "view logs to determine the cause of abnormal pipeline shutdown" | ||
| help_url: { $include: "health-report-pipeline-" } |
There was a problem hiding this comment.
Initial implementation was to validate make sure help_url exists when pipeline faces trouble(s).
It looks like we can extend this now to be more specific, like getting
health-report-pipeline-status(or flow):health-report-pipeline-recovery: https://github.com/elastic/logstash/pull/18930/changes#diff-08f220b4f357ed6c793e08230914dbacc05bf384fa8d9e302dfe8a26143f0743R368
Will you have another change with a new test which expects { $include: "health-report-pipeline-recovery" }? Otherwise, it looks like will be same with hard-coded logic.
There was a problem hiding this comment.
We can, but I'd prefer to keep that scope expansion out of this PR and follow up separately.
| if inclusion not in actual: | ||
| differences.append(f"Value at path `{path}` does not include:`{inclusion}`; got:`{actual}`") | ||
| # $match is a grok-like matcher that anchors the pattern at both ends | ||
| case {"$match": pattern_spec} if isinstance(expect, dict) and len(expect) == 1 and isinstance(actual, str): |
There was a problem hiding this comment.
It looks like we don't have match usage yet? Will you have use-case with your recovery change?
There was a problem hiding this comment.
Sounds good, your this suggestion clarifies the situation - https://github.com/elastic/logstash/pull/18930/changes#r3037810698
Co-authored-by: Mashhur <99575341+mashhurs@users.noreply.github.com>
💚 Build Succeeded
History
|
|
health tests with this branch: https://buildkite.com/elastic/logstash-health-report-tests-pipeline/builds/628/steps/waterfall build is green. |
3 participants
Release notes
[rn:skip]
What does this PR do?
The health check tests have a scenario validator that ties a specific validation for the only arrays in the current output to all arrays in the output, but pending changes introduce an array that won't satisfy it.
This refactors the validation logic to add a special
$includefor the substring-matching of that specific validation, and then uses it in all of the test expectations.It also introduces a special
$matchthat uses a grok-lite implementation for regexps or named patterns, along with a named pattern for ISO8601 that will be used in #18930.Why is it important/What is the impact to the user?
No user impact, but enables us to validate the health report API as it evolves.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)[ ] I have added tests that prove my fix is effective or that my feature worksAuthor's Checklist
mainand supported minorX.xbranches) -> buildHow to test this PR locally
See:
.buildkite/scripts/health-report-tests/README.md