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

[Endpoint] TEST: verify alerts page header says 'Alerts' #60206

Merged
merged 6 commits into from
Mar 16, 2020
Merged

[Endpoint] TEST: verify alerts page header says 'Alerts' #60206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8f1ba3c
test to verify alerts page header says alerts
EricDavisX Mar 14, 2020
98b6a6a
updating test with pr feedback
EricDavisX Mar 16, 2020
14205dc
updating test with pr feedback and better verbiage
EricDavisX Mar 16, 2020
7aebae8
Merge branch 'master' into alerts-list-tests
elasticmachine Mar 16, 2020
552b23a
updating test with pr feedback for better test titling
EricDavisX Mar 16, 2020
ce41ec2
Merge branch 'alerts-list-tests' of github.com:EricDavisX/kibana into…
EricDavisX Mar 16, 2020
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
2 changes: 1 addition & 1 deletion x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ export const AlertIndex = memo(() => {
<EuiPageContentHeader>
<EuiPageContentHeaderSection>
<EuiTitle size="l">
<h1>
<h1 data-test-subj="alertsViewTitle">
<FormattedMessage
id="xpack.endpoint.alertList.viewTitle"
defaultMessage="Alerts"
Expand Down
8 changes: 6 additions & 2 deletions x-pack/test/functional/apps/endpoint/alert_list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,20 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
const esArchiver = getService('esArchiver');
const browser = getService('browser');

describe('Endpoint Alert List', function() {
describe('Endpoint Alert List page', function() {
this.tags(['ciGroup7']);
before(async () => {
await esArchiver.load('endpoint/alerts/api_feature');
await pageObjects.common.navigateToUrlWithBrowserHistory('endpoint', '/alerts');
});

it('loads the Alert List Page', async () => {
it('loads in the browser', async () => {
await testSubjects.existOrFail('alertListPage');
});
it('contains the Alert List Page title', async () => {
const alertsTitle = await testSubjects.getVisibleText('alertsViewTitle');
expect(alertsTitle).to.equal('Alerts');
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to move it to a separate it block:

it('contains a page title', () => {});

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 can update this, but I disagree on the premise.

I have a separate discussion I'm going to post to our team and possibly the larger group about our test groupings. We're loading the page 3-5 times and thus far doing nothing interactive and there isn't even any data - it can be streamlined, and the tests run faster, if we choose to.

For now I'd propose we leave this as is, the word 'Alerts' is an integral part of the initial page load as the test case assesses. @peluja1012

Copy link
Contributor

Choose a reason for hiding this comment

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

It would seem that this expect should be in it's own it block based on how this describe block is already structured. Perhaps
it('includes Alerts title', ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It feels immaterial to me as I hope every page that loads has a title to be assessed. I can update it all the same, tho. Most importantly I am interested in driving the conversation further on test code structure best practices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i updated the test to a new 'it' block. i still disagree its needed or best, but I got 2 opinions on this that concurred, so its fine by me as its still fairly immaterial. Testrail updated to include a new test. Please re-review, @charlie-pichette @peluja1012

Copy link
Contributor

Choose a reason for hiding this comment

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

My reasoning for having it in a separate it() block was that it would be easier to determine on a failed test execution that the test failed because the Alerts title changed as opposed to the page not rendering at all. But I can also see the point that the title could be considered an integral part of the page loading.

});
it('includes alerts search bar', async () => {
await testSubjects.existOrFail('alertsSearchBar');
});
Expand Down