Skip to content
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

[EDR Workflows] Verify flaky osquery tests #169749

Merged
merged 57 commits into from
Nov 1, 2023

Conversation

tomsonpl
Copy link
Contributor

@tomsonpl tomsonpl commented Oct 25, 2023

  • Add video recording functionality to Osquery Cypress tests (only failed specs recording will be saved and uploaded as artifacts). Currently the video compressions is limited, because on full scale it was coming up to 100% of CPU usage.
  • remove usage of react-cypress-selector from packs_integration.cy.ts

Closes:

@tomsonpl tomsonpl added the release_note:skip Skip the PR/issue when compiling release notes label Oct 25, 2023
@tomsonpl tomsonpl self-assigned this Oct 25, 2023
cy.getBySel('policyIdsComboBox').should('exist');
cy.getBySel('osqueryPackTypeGlobal').click();
cy.getBySel('policyIdsComboBox').should('not.exist');

findAndClickButton('Save pack');
cy.get('button').contains('Save pack').click();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add data-test-subj instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can, let me try

cy.react('CustomItemAction', {
props: { index: 1, item: { id: 'users_elastic' } },
}).click();
cy.get(`[aria-label="Edit users_elastic"]`).click();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -80,9 +81,13 @@ Cypress.Commands.add(

Cypress.Commands.add('login', (role) => {
if (isServerless) {
cy.log('commands1');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, thanks 👍

import fs from 'fs';

// makes sure we save videos just for failed specs
export const filterCypressVideos = (spec: Cypress.Spec, results: CypressCommandLine.RunResult) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add more descriptive variable name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filteredFailedSpecVideos?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getFailedSpecVideos ?

@@ -6,10 +6,12 @@
*/

export const triggerLoadData = () => {
// @ts-expect-error update types for multiple true
const nodeContainers = cy.getBySel('nodeContainer', { multiple: true });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good practice to store the results of cy.getBySel in variable and used it later on

const hostname = agent.substring(0, 12);
await waitForHostToEnroll(kbnClient, hostname);
const agentTwo = await new AgentManager(policyEnrollmentKeyTwo, port, log).setup();
const hostnameTwo = agentTwo.substring(0, 12);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we return hostname from AgentManager, so we don't need to hardcode (0, 12)?


// cy.contains('Please fix issues listed below').should('not.exist');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove it?

@@ -87,6 +86,6 @@ describe.skip('ALL - Live Query', { tags: ['@ess', '@serverless'] }, () => {

inputQuery('{selectall}{backspace}{selectall}{backspace}');
// not sure if this is how it used to work when I implemented the functionality, but let's leave it like this for now
cy.get(LIVE_QUERY_EDITOR).invoke('height').should('be.gt', 200).and('be.lt', 380);
cy.get(LIVE_QUERY_EDITOR).invoke('height').should('be.gt', 200).and('be.lt', 400);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of hardcoding values, could we write it in a more dynamic way? checking if the values have changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah not sure, sometimes these numbers change, but in general it is good to know if it is in some scope, and not changed as from 300px to 3000px. I find this to be a quite easy way to get it tested, now just making the expectations more loose.

findAndClickButton('Save pack');

navigateToWithoutWaitForReact('app/osquery/packs');
cy.get('span').contains('Add pack').click();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think span are clickable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you think this works then? :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll use data-test-subj 👍

cy.get('span').contains('Add pack').click();
cy.get('input[name="name"]').type(`${REMOVING_PACK}{downArrow}{enter}`);
cy.getBySel('policyIdsComboBox').type(`${AGENT_POLICY_NAME}{downArrow}{enter}`);
// cy.get('button').contains('Save pack').click();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove commented out line

const port = config.get('servers.fleetserver.port');

const agent = await new AgentManager(policyEnrollmentKey, port, log).setup();
await waitForHostToEnroll(kbnClient, agent);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can move waitForHostToEnroll into AgentManager?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can, I had a different approach in place for firstly waitForHostToEnroll and if failed to create a new agent, but I do not do it now so this can be easily moved to agentManager if you think it makes sense

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.buildkite LGTM

@jbudz Hey, I added a video recording functionality to our cypress tests (saved only for failed specs), can you take a look from kibana-ops perspective, if this is fine ? Thanks!

ffmpeg is relatively cpu heavy. I see a few CPU spikes to 100%, but mostly in the 85% range. I'm okay with trying things out, but it's something to keep in mind if we start seeing timeouts.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
osquery 1.0MB 1.0MB +103.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @tomsonpl

@tomsonpl tomsonpl merged commit a15a948 into elastic:main Nov 1, 2023
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.11 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 169749

Questions ?

Please refer to the Backport tool documentation

@ashokaditya
Copy link
Member

💚 All backports created successfully

Status Branch Result
8.11

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

ashokaditya pushed a commit to ashokaditya/kibana that referenced this pull request Nov 7, 2023
(cherry picked from commit a15a948)

# Conflicts:
#	.buildkite/scripts/lifecycle/post_command.sh
#	x-pack/plugins/osquery/cypress/e2e/all/alerts_automated_action_results.cy.ts
#	x-pack/plugins/osquery/cypress/e2e/all/alerts_response_actions_form.cy.ts
#	x-pack/plugins/osquery/cypress/e2e/all/packs_integration.cy.ts
#	x-pack/plugins/osquery/cypress/support/e2e.ts
tomsonpl added a commit to tomsonpl/kibana that referenced this pull request Nov 9, 2023
(cherry picked from commit a15a948)

# Conflicts:
#	.buildkite/scripts/lifecycle/post_command.sh
#	x-pack/plugins/osquery/cypress/e2e/all/alerts_automated_action_results.cy.ts
#	x-pack/plugins/osquery/cypress/e2e/all/alerts_response_actions_form.cy.ts
#	x-pack/plugins/osquery/cypress/e2e/all/packs_integration.cy.ts
#	x-pack/plugins/osquery/cypress/support/e2e.ts
@tomsonpl
Copy link
Contributor Author

tomsonpl commented Nov 9, 2023

💚 All backports created successfully

Status Branch Result
8.11

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Osquery Security Solution Osquery feature release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.11.0 v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants