-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Security Solution][Explore] Fixes failing test in MKI environments #186116
[Security Solution][Explore] Fixes failing test in MKI environments #186116
Conversation
/ci |
/ci |
/ci |
Pinging @elastic/security-threat-hunting-explore (Team:Threat Hunting:Explore) |
@@ -42,5 +43,17 @@ export const samlAuthentication = async ( | |||
); | |||
return sessionManager.getSessionCookieForRole(role); | |||
}, | |||
getFullname: async (role: string | SecurityRoleName = 'platform_engineer'): Promise<string> => { |
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.
Do we have the default serverless role defined as a constant somewhere? Otherwise, we are also hardcoding the platform_engineer
here
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.
No, but we can do it.
Seems that the changes introduced in the following PR #186279 is affecting this one so I need to touch the code to make it work again. |
Changes done and seems to be working fine :) |
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.
Thanks you @MadameSheema! I have reviewed and left a couple questions, but overall PR LGTM!
const isServerless = Cypress.env(IS_SERVERLESS); | ||
const username = isServerless ? 'platform_engineer' : Cypress.env(ELASTICSEARCH_USERNAME); | ||
const getUsername = () => { |
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.
This looks like something that is going to be reused in the future. Should we convert this into a Cypress command, like cy.getUsername()
?
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 put it there since I saw that only this test was using that functionality.
const username = isServerless ? 'platform_engineer' : Cypress.env(ELASTICSEARCH_USERNAME); | ||
const getUsername = () => { | ||
if (isServerless) { | ||
return cy.task('getFullname'); |
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 am not familiar with this part, but curious why do we get the full name and not the username. Is it not available in Serverless?
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 was using code that was already available so I don't know which is the correct answer for that.
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!
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.
👍🏼
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Fix: #179231
##Summary
When we changed the default role used in Cypress from
admin
toplatform_engineer
, thex-pack/test/security_solution_cypress/cypress/e2e/explore/cases/creation.cy.ts
test started to fail in MKI environments.The test was failing in both assertions for MKI environments:
cy.get(CASE_DETAILS_USERNAMES).eq(REPORTER).should('contain', username);
cy.get(CASE_DETAILS_USERNAMES).eq(PARTICIPANTS).should('contain', username);
This was happening because we were hardcoding the username and this is a thing we should not be doing anymore since we are making the test prone to failures.
In this PR we are fixing that behavior by using a method that retrieves the correct value.