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

[Reporting] Unit test for screenshot observable #57638

Merged
merged 9 commits into from
Feb 18, 2020

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Feb 13, 2020

This adds a suite of unit tests to the screenshotsObservableFactory method of Kibana Reporting.

Related to the fix from #56928

@tsullivan tsullivan force-pushed the reporting/test-screenshot-pipe branch from 5511db0 to 133bcf6 Compare February 14, 2020 21:20
commit 1c6a6b0
Author: Timothy Sullivan <[email protected]>
Date:   Thu Feb 13 14:10:40 2020 -0700

    fix tests

commit a3afefd
Merge: facd94b 6e4efdf
Author: Timothy Sullivan <[email protected]>
Date:   Thu Feb 13 13:56:24 2020 -0700

    Merge branch 'master' into reporting/np-server-uisettings

commit facd94b
Author: Timothy Sullivan <[email protected]>
Date:   Thu Feb 13 12:16:51 2020 -0700

    extract internals access to separate core class

commit 2f12495
Merge: dd0fd40 3f3969d
Author: Timothy Sullivan <[email protected]>
Date:   Thu Feb 13 09:57:07 2020 -0700

    Merge branch 'master' into reporting/np-server-uisettings

commit dd0fd40
Author: Timothy Sullivan <[email protected]>
Date:   Wed Feb 12 14:44:03 2020 -0700

    add more tests

commit 25dc761
Author: Timothy Sullivan <[email protected]>
Date:   Wed Feb 12 13:54:47 2020 -0700

    simplify reporting usage collector setup

commit e8aa02d
Author: Timothy Sullivan <[email protected]>
Date:   Wed Feb 12 13:44:13 2020 -0700

    consistent name for reportingPlugin

commit d235b16
Author: Timothy Sullivan <[email protected]>
Date:   Wed Feb 12 13:52:07 2020 -0700

    Prettier changes

commit e8ddc7b
Author: Timothy Sullivan <[email protected]>
Date:   Wed Feb 12 13:28:35 2020 -0700

    [Reporting/New Platform] Provide async access to server-side
@tsullivan tsullivan force-pushed the reporting/test-screenshot-pipe branch from 133bcf6 to 24ea6b3 Compare February 16, 2020 19:13
@tsullivan tsullivan added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v7.7.0 v8.0.0 labels Feb 18, 2020
@tsullivan tsullivan requested review from joelgriffith and a team February 18, 2020 17:24
@tsullivan tsullivan marked this pull request as ready for review February 18, 2020 17:24
@tsullivan tsullivan added the release_note:skip Skip the PR/issue when compiling release notes label Feb 18, 2020
@@ -4,48 +4,55 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { HeadlessChromiumDriver as HeadlessBrowser } from '../../../../server/browsers/chromium/driver';
import { HeadlessChromiumDriver as HeadlessBrowser } from '../../../../server/browsers';
Copy link
Member Author

Choose a reason for hiding this comment

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

This one, and the next 3 files have a lot of whitespace changes enforced by prettier. It would help to ignore whitespace changes in this PR review :)

}).toPromise();
};

await expect(getScreenshot()).rejects.toMatchInlineSnapshot(
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: This part of assertions will change when #57008 is implemented

@@ -0,0 +1,129 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Whew this is a fun one

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah.. I think this test and the way things are mocked, especially in createPage highlight some overly-complicated areas of Reporting and maybe things that have real performance impact.

Copy link
Member Author

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

need to restore skip_telemetry

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Looks good to me

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@tsullivan tsullivan merged commit c47612f into elastic:master Feb 18, 2020
@tsullivan tsullivan deleted the reporting/test-screenshot-pipe branch February 18, 2020 23:21
tsullivan added a commit to tsullivan/kibana that referenced this pull request Feb 18, 2020
[Reporting] test the screenshot observable
tsullivan added a commit that referenced this pull request Feb 19, 2020
[Reporting] test the screenshot observable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants