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

EricDavisX · 2020-03-14T19:57:19Z

Summary

Adds in a test to verify that the word 'Alerts' is the Alert header for the page. other pages have it so it seemed a silly easy way to do a first PR as I explore Typescript and our repo and code.

Checklist

n/a

For maintainers

n/a

peluja1012 · 2020-03-16T10:07:08Z

x-pack/test/functional/apps/endpoint/alert_list.ts

@@ -21,6 +21,8 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {

    it('loads the Alert List Page', async () => {
      await testSubjects.existOrFail('alertListPage');
+      const alertsTitle = await testSubjects.getVisibleText('alertsViewTitle');
+      expect(alertsTitle).to.equal('Alerts');


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

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

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

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', ...

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.

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

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.

elasticmachine · 2020-03-16T10:07:32Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

elasticmachine · 2020-03-16T10:07:44Z

Pinging @elastic/endpoint-response (Team:Endpoint Response)

EricDavisX · 2020-03-16T17:59:50Z

@elasticmachine merge upstream

EricDavisX · 2020-03-16T18:12:50Z

Thanks Pedro. I acknowledge it would be helpful to know, its good to split them, as is done.

And Charlie urged me to better assess the page logins/reloads and I found it wasn't due to the number of tests anyhow, so the overall test time increase concern isn't / wasn't valid.

peluja1012 · 2020-03-16T18:33:29Z

x-pack/test/functional/apps/endpoint/alert_list.ts

@@ -22,6 +22,10 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
    it('loads the Alert List Page', async () => {
      await testSubjects.existOrFail('alertListPage');
    });
+    it('verifies the Alert List Page title', async () => {


The way this test description is written will cause the test execution to read like this:

Endpoint Alert List verifies the Alert List Page title

I suggest updating the description such that the execution read better. Maybe something like:

Endpoint Alert List contains the Alerts title

it('contains the Alerts title', async () => {});

That's better than what I have, will update shortly. Thanks P.

i modified, lets see how we like the latest when folks have time to review

… alerts-list-tests

peluja1012

Looks good. Thanks!

kibanamachine · 2020-03-16T20:35:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ce41ec2

History

💚 Build #33531 succeeded 7aebae8
💛 Build #33273 was flaky 8f1ba3c

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

* master: (30 commits) [TSVB] fix text color when using custom background color (elastic#60261) Fix import to timefilter from in TSVB (elastic#60296) [NP] Get rid of usage redirectWhenMissing service (elastic#59777) [SIEM] Fix Timeline footer styling (elastic#59587) [ML] Fixes to error handling for analytics jobs and file data viz (elastic#60249) Give better stack traces for Unhandled Promise Rejection warnings (elastic#60235) resolves elastic#58905 (elastic#60120) Added variables button for text fields in Pagerduty component. (elastic#60189) adds test that action vars are rendered for alert action parms (elastic#60310) Closes 59786 by removing the update toast (elastic#60172) [EPM] Packages list tabs (elastic#60167) Added message variables button for Webhook body form field (elastic#60174) Revert "adds new test (elastic#60064)" [Maps] move MapSavedObject type out of telemetry (elastic#60127) [Reporting] Fix error handling for job handler in route (elastic#60161) [Endpoint] TEST: verify alerts page header says 'Alerts' (elastic#60206) EMT-248: implement ack resource to accept event payload to acknowledge agent actions (elastic#60218) Migrate dual validated range (elastic#59689) Embeddable triggers (elastic#58440) [Endpoint] Sample data generator CLI script (elastic#59952) ...

* test to verify alerts page header says alerts * updating test with pr feedback * updating test with pr feedback and better verbiage * updating test with pr feedback for better test titling Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kibanamachine · 2020-03-17T21:28:44Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2020-03-18T21:38:43Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

spalger · 2020-03-19T00:35:06Z

7.x/7.7: e29c401

test to verify alerts page header says alerts

8f1ba3c

EricDavisX requested a review from a team as a code owner March 14, 2020 19:57

EricDavisX added v7.7.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 15, 2020

EricDavisX changed the title ~~TEST: test to verify alerts page header says alerts~~ TEST: verify alerts page header says 'Alerts' Mar 15, 2020

peluja1012 reviewed Mar 16, 2020

View reviewed changes

peluja1012 added the Feature:Endpoint Elastic Endpoint feature label Mar 16, 2020

peluja1012 added the Team:Endpoint Response Endpoint Response Team label Mar 16, 2020

EricDavisX added 2 commits March 16, 2020 13:53

updating test with pr feedback

98b6a6a

updating test with pr feedback and better verbiage

14205dc

Merge branch 'master' into alerts-list-tests

7aebae8

peluja1012 reviewed Mar 16, 2020

View reviewed changes

EricDavisX added 2 commits March 16, 2020 14:53

updating test with pr feedback for better test titling

552b23a

Merge branch 'alerts-list-tests' of github.com:EricDavisX/kibana into…

ce41ec2

… alerts-list-tests

peluja1012 changed the title ~~TEST: verify alerts page header says 'Alerts'~~ [Endpoint] TEST: verify alerts page header says 'Alerts' Mar 16, 2020

peluja1012 approved these changes Mar 16, 2020

View reviewed changes

EricDavisX merged commit 132383c into elastic:master Mar 16, 2020

EricDavisX deleted the alerts-list-tests branch March 16, 2020 21:25

EricDavisX mentioned this pull request Mar 17, 2020

[7.x] [Endpoint] TEST: verify alerts page header says 'Alerts' (#60206) #60394

Merged

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 17, 2020

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

EricDavisX commented Mar 14, 2020

peluja1012 Mar 16, 2020

EricDavisX Mar 16, 2020

charlie-pichette Mar 16, 2020

EricDavisX Mar 16, 2020

EricDavisX Mar 16, 2020

peluja1012 Mar 16, 2020

elasticmachine commented Mar 16, 2020

elasticmachine commented Mar 16, 2020

EricDavisX commented Mar 16, 2020

EricDavisX commented Mar 16, 2020

peluja1012 Mar 16, 2020 •

edited

Loading

EricDavisX Mar 16, 2020

EricDavisX Mar 16, 2020

peluja1012 left a comment

kibanamachine commented Mar 16, 2020

kibanamachine commented Mar 17, 2020

kibanamachine commented Mar 18, 2020

spalger commented Mar 19, 2020

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

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

Conversation

EricDavisX commented Mar 14, 2020

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 16, 2020

elasticmachine commented Mar 16, 2020

EricDavisX commented Mar 16, 2020

EricDavisX commented Mar 16, 2020

peluja1012 Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peluja1012 left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 16, 2020

💚 Build Succeeded

History

kibanamachine commented Mar 17, 2020

kibanamachine commented Mar 18, 2020

spalger commented Mar 19, 2020

peluja1012 Mar 16, 2020 •

edited

Loading