-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Cloud Security][FTR]Refactor API FTR to use .to.eql instead of .to.be #160694
Conversation
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
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.
Hi I added comments I'd like you to apply across all the assertions
Really useful refactoring! thanks ricky
x-pack/test/api_integration/apis/cloud_security_posture/status/status_unprivileged.ts
Show resolved
Hide resolved
x-pack/test/api_integration/apis/cloud_security_posture/status/status_unprivileged.ts
Show resolved
Hide resolved
x-pack/test/api_integration/apis/cloud_security_posture/status/status_unprivileged.ts
Show resolved
Hide resolved
x-pack/test/api_integration/apis/cloud_security_posture/status/status_unprivileged.ts
Outdated
Show resolved
Hide resolved
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.
Suggested another refactoring and to add index name into the assertion message
expect( | ||
res.indicesDetails.find((idx) => idx.index === LATEST_VULNERABILITIES_INDEX_DEFAULT_NS) | ||
?.status | ||
).to.eql( | ||
'empty', | ||
`expected empty but got ${ | ||
res.indicesDetails.find((idx) => idx.index === LATEST_VULNERABILITIES_INDEX_DEFAULT_NS) | ||
?.status | ||
} instead` | ||
); |
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 suggest adding the checked index to the message
expect( | |
res.indicesDetails.find((idx) => idx.index === LATEST_VULNERABILITIES_INDEX_DEFAULT_NS) | |
?.status | |
).to.eql( | |
'empty', | |
`expected empty but got ${ | |
res.indicesDetails.find((idx) => idx.index === LATEST_VULNERABILITIES_INDEX_DEFAULT_NS) | |
?.status | |
} instead` | |
); | |
expect( | |
res.indicesDetails.find((idx) => idx.index === LATEST_VULNERABILITIES_INDEX_DEFAULT_NS) | |
?.status | |
).to.eql( | |
'empty', | |
`expected ${LATEST_VULNERABILITIES_INDEX_DEFAULT_NS} empty but got ${ | |
res.indicesDetails.find((idx) => idx.index === LATEST_VULNERABILITIES_INDEX_DEFAULT_NS) | |
?.status | |
} instead` | |
); |
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.
And to clean up the code you can create the following assertion func:
const assertIndexStatus(indicesDetails, indexName, expectedStatus) => {
const actualStatus = indicesDetails.find(idx => idx.name === indexName)?.status);
expect(actualStatus).to.eql(expectedStatus, `expected ${indexName} to be ${expectedStatus} but got ${actualStatus} instead`);
}
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.
And use it
assertIndexStatus(res.indicesDetails, LATEST_VULNERABILITIES_INDEX_DEFAULT_NS, 'empty');
…-ref HEAD~1..HEAD --fix'
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.
LGTM,
suggested a fix to assertIndexStatus message
x-pack/test/api_integration/apis/cloud_security_posture/helper.ts
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
* main: (354 commits) [Synthetics] Overview page fix last refresh value display (elastic#161086) [Synthetics] Remove TLS alert option for ICMP monitor (elastic#161173) fixing the path of manifets for hints autodiscover (elastic#161075) [Fleet] Fix permissions in integrations Assets page (elastic#161233) Update publicBaseUrl warning id (elastic#161204) [ML] Fix Anomaly Explorer URL for alerting context with non-default space (elastic#160899) [Enterprise Search]Add 404 error handling for mappings and documents endpoints (elastic#161203) [Logs Shared] Move LogStream and LogView into new shared plugin (elastic#161151) [Security Solutions] Fix CellActions component should hide ShowTopN action for nested fields (elastic#159645) [SecuritySolutions] Remove filter actions from Cases alerts table and fix show_top_n action (elastic#161150) [Infrastructure UI] Add strict payload validation to inventory_views endpoint (elastic#160852) [api-docs] 2023-07-05 Daily api_docs build (elastic#161225) Fix errors in custom metric payload in SLO dev docs (elastic#161141) [data views] Fix overwrite param for create (elastic#160953) [Synthetics] Perform params API HTTP migration (elastic#160575) [Cloud Security][FTR]Refactor API FTR to use .to.eql instead of .to.be (elastic#160694) Have SLO routes return a 403 instead of a 400 when user has an insufficient license (elastic#161193) [Discover] Fix shared links flaky test (elastic#161172) [ftr] Improve FTR error handling for NoSuchSessionError (elastic#161025) skip flaky suite (elastic#151981) ...
* main: (354 commits) [Synthetics] Overview page fix last refresh value display (elastic#161086) [Synthetics] Remove TLS alert option for ICMP monitor (elastic#161173) fixing the path of manifets for hints autodiscover (elastic#161075) [Fleet] Fix permissions in integrations Assets page (elastic#161233) Update publicBaseUrl warning id (elastic#161204) [ML] Fix Anomaly Explorer URL for alerting context with non-default space (elastic#160899) [Enterprise Search]Add 404 error handling for mappings and documents endpoints (elastic#161203) [Logs Shared] Move LogStream and LogView into new shared plugin (elastic#161151) [Security Solutions] Fix CellActions component should hide ShowTopN action for nested fields (elastic#159645) [SecuritySolutions] Remove filter actions from Cases alerts table and fix show_top_n action (elastic#161150) [Infrastructure UI] Add strict payload validation to inventory_views endpoint (elastic#160852) [api-docs] 2023-07-05 Daily api_docs build (elastic#161225) Fix errors in custom metric payload in SLO dev docs (elastic#161141) [data views] Fix overwrite param for create (elastic#160953) [Synthetics] Perform params API HTTP migration (elastic#160575) [Cloud Security][FTR]Refactor API FTR to use .to.eql instead of .to.be (elastic#160694) Have SLO routes return a 403 instead of a 400 when user has an insufficient license (elastic#161193) [Discover] Fix shared links flaky test (elastic#161172) [ftr] Improve FTR error handling for NoSuchSessionError (elastic#161025) skip flaky suite (elastic#151981) ...
Summary
This PR is for refactoring current API FTR to use .to.eql instead of .to.be for more understandable error message when an error occurs. Currently when test fail due to unmatched value, the error message can be quite confusing as we don't know which one is the expected and actual value from current error message.
With this change from this PR it's easier to see which is the expected value and which is the actual value