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.

Sign up for GitHub

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

[SecuritySolution] Unskip fields browser timeline tests #174597

Merged
merged 12 commits into from
Jan 12, 2024
Merged

[SecuritySolution] Unskip fields browser timeline tests #174597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1895256
context -> describe
janmonschke Jan 10, 2024
91a2073
remove force clicks/checks/unchecks
janmonschke Jan 10, 2024
04ddc10
remove unnecessary calls to openTimelineFieldsBrowser
janmonschke Jan 10, 2024
d5140f7
Remove populateTimeline
janmonschke Jan 10, 2024
c8cf895
prepare flaky test runner
janmonschke Jan 10, 2024
42d1970
undo flaky test runner changes
janmonschke Jan 10, 2024
57eeecc
replace part of the acceptance test with a unit test
janmonschke Jan 10, 2024
92e3917
Merge branch 'main' into security/unskip-fields-browser-timeline-tests
janmonschke Jan 10, 2024
04390a0
simplify tests
janmonschke Jan 11, 2024
2c76383
simplify tests
janmonschke Jan 11, 2024
8e9520f
rename 2 tests to use should... convention
PhilippeOberti Jan 11, 2024
9f97807
Merge branch 'main' into security/unskip-fields-browser-timeline-tests
PhilippeOberti Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...gins/triggers_actions_ui/public/application/sections/field_browser/field_browser.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,12 @@ describe('FieldsBrowser', () => {

result.getByTestId('show-field-browser').click();
await waitFor(() => {
// the container is rendered now
expect(result.getByTestId('fields-browser-container')).toBeInTheDocument();
// by default, no categories are selected
expect(result.getByTestId('category-badges')).toHaveTextContent('');
// the view: all button is shown by default
result.getByText('View: all');
});
});
});
Expand Down
95 changes: 31 additions & 64 deletions .../test/security_solution_cypress/cypress/e2e/investigations/timelines/fields_browser.cy.ts
View file
Open in desktop
janmonschke marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ import {
FIELDS_BROWSER_MESSAGE_HEADER,
FIELDS_BROWSER_FILTER_INPUT,
FIELDS_BROWSER_CATEGORIES_FILTER_CONTAINER,
FIELDS_BROWSER_SELECTED_CATEGORIES_BADGES,
FIELDS_BROWSER_CATEGORY_BADGE,
FIELDS_BROWSER_VIEW_BUTTON,
} from '../../../screens/fields_browser';
import { TIMELINE_FIELDS_BUTTON } from '../../../screens/timeline';

Expand All @@ -29,12 +27,11 @@ import {
resetFields,
toggleCategory,
activateViewSelected,
activateViewAll,
} from '../../../tasks/fields_browser';
import { login } from '../../../tasks/login';
import { visitWithTimeRange } from '../../../tasks/navigation';
import { openTimelineUsingToggle } from '../../../tasks/security_main';
import { openTimelineFieldsBrowser, populateTimeline } from '../../../tasks/timeline';
import { openTimelineFieldsBrowser } from '../../../tasks/timeline';

import { hostsUrl } from '../../../urls/navigation';

Expand All @@ -49,110 +46,83 @@ const defaultHeaders = [
{ id: 'user.name' },
];

// Flaky in serverless tests
// FLAKY: https://github.com/elastic/kibana/issues/169363
describe.skip('Fields Browser', { tags: ['@ess', '@serverless'] }, () => {
context('Fields Browser rendering', () => {
beforeEach(() => {
login();
visitWithTimeRange(hostsUrl('allHosts'));
openTimelineUsingToggle();
populateTimeline();
openTimelineFieldsBrowser();
});

it('displays all categories (by default)', () => {
cy.get(FIELDS_BROWSER_SELECTED_CATEGORIES_BADGES).should('be.empty');
});

it('displays "view all" option by default', () => {
cy.get(FIELDS_BROWSER_VIEW_BUTTON).should('contain.text', 'View: all');
});
describe('Fields Browser', { tags: ['@ess', '@serverless'] }, () => {
beforeEach(() => {
login();
visitWithTimeRange(hostsUrl('allHosts'));
openTimelineUsingToggle();
openTimelineFieldsBrowser();
});

it('displays the expected count of categories that match the filter input', () => {
describe('Fields Browser rendering', () => {
it('should display the expected count of categories and fields that match the filter input', () => {
const filterInput = 'host.mac';

filterFieldsBrowser(filterInput);

cy.get(FIELDS_BROWSER_CATEGORIES_COUNT).should('have.text', '2');
});

it('displays a search results label with the expected count of fields matching the filter input', () => {
const filterInput = 'host.mac';
filterFieldsBrowser(filterInput);

cy.get(FIELDS_BROWSER_FIELDS_COUNT).should('contain.text', '2');
});

it('displays only the selected fields when "view selected" option is enabled', () => {
it('should display only the selected fields when "view selected" option is enabled', () => {
activateViewSelected();
cy.get(FIELDS_BROWSER_FIELDS_COUNT).should('contain.text', `${defaultHeaders.length}`);
defaultHeaders.forEach((header) => {
cy.get(`[data-test-subj="field-${header.id}-checkbox"]`).should('be.checked');
});
activateViewAll();
});

it('creates the category badge when it is selected', () => {
it('should create the category badge when it is selected', () => {
const category = 'host';
const categoryCheck = 'event';

cy.get(FIELDS_BROWSER_CATEGORY_BADGE(category)).should('not.exist');

toggleCategory(category);

cy.get(FIELDS_BROWSER_CATEGORY_BADGE(category)).should('exist');

toggleCategory(category);
});

it('search a category should match the category in the category filter', () => {
const category = 'host';
cy.log('the category filter should contain the filtered category');

filterFieldsBrowser(category);
toggleCategoryFilter();

cy.get(FIELDS_BROWSER_CATEGORIES_FILTER_CONTAINER).should('contain.text', category);
});

it('search a category should filter out non matching categories in the category filter', () => {
const category = 'host';
const categoryCheck = 'event';
filterFieldsBrowser(category);
toggleCategoryFilter();
cy.log('non-matching categories should not be listed in the category filter');

cy.get(FIELDS_BROWSER_CATEGORIES_FILTER_CONTAINER).should('not.contain.text', categoryCheck);
});
});

context('Editing the timeline', () => {
beforeEach(() => {
login();
visitWithTimeRange(hostsUrl('allHosts'));
openTimelineUsingToggle();
populateTimeline();
openTimelineFieldsBrowser();
});
describe('Editing the timeline', () => {
it('should add/remove columns from the alerts table when the user checks/un-checks them', () => {
const filterInput = 'host.geo.c';

cy.log('removing the message column');

Copy link
Contributor

Choose a reason for hiding this comment

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

same comment here, I feel like the first 3 tests could easily be merged into a single one that would test add, remove and reset fields.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually even the fourth one (restores focus to the Customize Columns button when Reset Fields is clicked) can easily be merged with the other 3. Just check the focus after you've reset. It would just be a this single line added at the end

cy.get(TIMELINE_FIELDS_BUTTON).should('have.focus');

It doesn't really make sense to have a whole test that verifies just that

it('removes the message field from the timeline when the user un-checks the field', () => {
cy.get(FIELDS_BROWSER_MESSAGE_HEADER).should('exist');

removesMessageField();
closeFieldsBrowser();

cy.get(FIELDS_BROWSER_MESSAGE_HEADER).should('not.exist');
});

it('adds a field to the timeline when the user clicks the checkbox', () => {
const filterInput = 'host.geo.c';
cy.log('add host.geo.city_name column');

closeFieldsBrowser();
cy.get(FIELDS_BROWSER_HOST_GEO_CITY_NAME_HEADER).should('not.exist');

openTimelineFieldsBrowser();

filterFieldsBrowser(filterInput);
addsHostGeoCityNameToTimeline();
closeFieldsBrowser();

cy.get(FIELDS_BROWSER_HOST_GEO_CITY_NAME_HEADER).should('exist');
});

it('resets all fields in the timeline when `Reset Fields` is clicked', () => {
it('should reset all fields in the timeline when `Reset Fields` is clicked', () => {
const filterInput = 'host.geo.c';

filterFieldsBrowser(filterInput);
Expand All @@ -168,19 +138,16 @@ describe.skip('Fields Browser', { tags: ['@ess', '@serverless'] }, () => {
resetFields();

cy.get(FIELDS_BROWSER_HEADER_HOST_GEO_CONTINENT_NAME_HEADER).should('not.exist');
});

it('restores focus to the Customize Columns button when `Reset Fields` is clicked', () => {
openTimelineFieldsBrowser();
resetFields();
cy.log('restores focus to the Customize Columns button when `Reset Fields` is clicked');

cy.get(TIMELINE_FIELDS_BUTTON).should('have.focus');
});

it('restores focus to the Customize Columns button when Esc is pressed', () => {
cy.log('restores focus to the Customize Columns button when Esc is pressed');

openTimelineFieldsBrowser();
cy.get(FIELDS_BROWSER_FILTER_INPUT).type('{esc}');

cy.get(FIELDS_BROWSER_FILTER_INPUT).type('{esc}');
cy.get(TIMELINE_FIELDS_BUTTON).should('have.focus');
});
});
Expand Down
24 changes: 9 additions & 15 deletions x-pack/test/security_solution_cypress/cypress/tasks/fields_browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,11 @@ export const addsFields = (fields: string[]) => {
};

export const addsHostGeoCityNameToTimeline = () => {
cy.get(FIELDS_BROWSER_HOST_GEO_CITY_NAME_CHECKBOX).check({
force: true,
});
cy.get(FIELDS_BROWSER_HOST_GEO_CITY_NAME_CHECKBOX).check();
};

export const addsHostGeoContinentNameToTimeline = () => {
cy.get(FIELDS_BROWSER_HOST_GEO_CONTINENT_NAME_CHECKBOX).check({
force: true,
});
cy.get(FIELDS_BROWSER_HOST_GEO_CONTINENT_NAME_CHECKBOX).check();
};

export const clearFieldsBrowser = () => {
Expand Down Expand Up @@ -67,7 +63,7 @@ export const filterFieldsBrowser = (fieldName: string) => {
};

export const toggleCategoryFilter = () => {
cy.get(FIELDS_BROWSER_CATEGORIES_FILTER_BUTTON).click({ force: true });
cy.get(FIELDS_BROWSER_CATEGORIES_FILTER_BUTTON).click();
};

export const toggleCategory = (category: string) => {
Expand All @@ -79,24 +75,22 @@ export const toggleCategory = (category: string) => {
};

export const removesMessageField = () => {
cy.get(FIELDS_BROWSER_MESSAGE_CHECKBOX).uncheck({
force: true,
});
cy.get(FIELDS_BROWSER_MESSAGE_CHECKBOX).uncheck();
};

export const removeField = (fieldName: string) => {
cy.get(GET_FIELD_CHECKBOX(fieldName)).uncheck();
};

export const resetFields = () => {
cy.get(FIELDS_BROWSER_RESET_FIELDS).click({ force: true });
cy.get(FIELDS_BROWSER_RESET_FIELDS).click();
};

export const activateViewSelected = () => {
cy.get(FIELDS_BROWSER_VIEW_BUTTON).click({ force: true });
cy.get(FIELDS_BROWSER_VIEW_SELECTED).click({ force: true });
cy.get(FIELDS_BROWSER_VIEW_BUTTON).click();
cy.get(FIELDS_BROWSER_VIEW_SELECTED).click();
};
export const activateViewAll = () => {
cy.get(FIELDS_BROWSER_VIEW_BUTTON).click({ force: true });
cy.get(FIELDS_BROWSER_VIEW_ALL).click({ force: true });
cy.get(FIELDS_BROWSER_VIEW_BUTTON).click();
cy.get(FIELDS_BROWSER_VIEW_ALL).click();
};
2 changes: 1 addition & 1 deletion x-pack/test/security_solution_cypress/cypress/tasks/timeline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ export const markAsFavorite = () => {
};

export const openTimelineFieldsBrowser = () => {
cy.get(TIMELINE_FIELDS_BUTTON).first().click({ force: true });
cy.get(TIMELINE_FIELDS_BUTTON).first().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 just want to note that the first() here is pretty dangerous/brittle. I was trying to remove it and realized that we have 3 instances of the FieldBrowserComponent which contains the EuiButtonEmpty that has the data-test-subj="show-field-browser" property.
It happens that in the DOM the first one is the one that we mostly want, but that could change.

I don't think this PR is the right place to do it, so this is just a note, but we should at some point modify the FieldBrowserComponent to take the `data-test-subj- as a prop so we can guarantee the uniqueness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, first() often is a source of flakiness/trouble in the future. I wouldn't want to touch it in this PR. Also, it seems like the component is owned by ResponseOps and not us...

};

export const openTimelineInspectButton = () => {
Expand Down